branch-4.1: [fix](thirdparty) order Paimon codec dependencies #67158 - #67357
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review status: complete after 2 convergence rounds on 2e5d3d795c2f56bffd6acaa5385313a32c7bc12a. I am requesting changes for one P1 prebuilt-compatibility regression and one P2 regression-guard defect.
Critical checkpoint conclusions
- Goal and proof: The Paimon codec-ordering hunk correctly makes
paimon_objlibwait for ZSTD, Snappy, and LZ4 installation, but the stated prebuilt-reuse goal is not achieved because the immediately previous artifact-equivalentcb82e41b...marker is rejected. - Scope and clarity: The six-file backport is focused and understandable. The two remaining issues are isolated to Paimon fingerprint migration and the new ADBC config guard.
- Concurrency and lifecycle: Exact patched-source Ninja and Make graphs across external/bundled Arrow and static/shared combinations prove all three codec ExternalProject install steps precede object compilation without a cycle. Source reset, marker invalidation, artifact validation, and publication were traced; the missing compatibility entry is the remaining lifecycle break for install-only prefixes.
- Configuration and compatibility:
${DORIS_HOME}/libhas the correct directory semantics for Arrow ADBC and expands correctly at FE startup. The ADBC path is currently dormant on branch-4.1; the inline issue concerns the newly added guard falsely accepting a superseded assignment. The Paimon marker rejection is active compatibility behavior and can prevent valid build-image prebuilts from being used. - Parallel paths and platforms: External/bundled Arrow, static/shared outputs, Linux/Darwin branches, Ninja/Make, workflow routing, and source/prebuilt recovery were covered. The live macOS-arm64 failure is unrelated: the unchanged libunwind patch fails before Paimon is reached.
- Tests and results:
bash -nand all three focused scripts pass locally, and the live Arrow/Paimon lifecycle job passes. Those checks do not cover the missingcb82...acceptance or final-assignment precedence. No builds were run in this review environment, per the review constraints. - Error handling and observability: Workflow and shell failures propagate with actionable messages. No additional runtime logging or metrics are warranted for these build/configuration changes.
- Persistence, transactions, data correctness, memory accounting, security, and FE/BE protocol: Not applicable; this diff changes third-party build ordering/selection metadata, CI guards, and an external FE JVM option.
- Performance: No runtime performance impact was found. The new target prerequisites constrain only necessary build order and retain concurrency among independent codec projects.
- Completion: Round 2's two normal reviews and separate risk-focused review all returned
NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, duplicated, or dismissed with concrete evidence.
User focus
No additional user-provided review focus was specified; the complete PR was reviewed.
| PAIMON_LEGACY_COMPATIBLE_SEMANTIC_FINGERPRINT="cb82e41ba46f534e611cdd52e66b53c227d49bf8" | ||
| # Dependency ordering does not change the installed Paimon artifacts, so existing | ||
| # complete prebuilts remain compatible with the corrected clean-build graph. | ||
| PAIMON_LEGACY_COMPATIBLE_SEMANTIC_FINGERPRINT="9887cf1ec13106da4250f8d7ea931a258879089c" |
There was a problem hiding this comment.
[P1] Keep the immediately previous Paimon marker reusable
This rotates the selected-input gate to 9887cf1e..., but PAIMON_LEGACY_BUILD_FINGERPRINTS still contains only dbb6ca6e..., so the current matcher rejects a complete prebuilt stamped by the base inputs (cb82e41b...). Because this patch only changes dependency ordering, those artifacts remain equivalent as the comment says; nevertheless build.sh sends such prefixes to recovery, which fails for install-only build-image prefixes. Please add cb82e41ba46f534e611cdd52e66b53c227d49bf8 to the legacy list and cover that marker in the lifecycle test.
|
|
||
| # External Regression replaces the packaged FE configuration instead of merging it. | ||
| # Keep the JNI override in that replacement so ADBC cannot fall back to the jar binary. | ||
| java_opts_assignment="$(grep '^JAVA_OPTS_FOR_JDK_17=' "${EXTERNAL_FE_CONF}" || true)" |
There was a problem hiding this comment.
[P2] Check the runtime-effective JDK option assignment
start_fe.sh processes every uppercase assignment in order, so a later duplicate JAVA_OPTS_FOR_JDK_17 line overrides an earlier one. This grep concatenates all matching lines and the following substring check passes if any earlier assignment contains the JNI property; a later effective assignment can therefore drop the option while this new regression guard stays green. Please either require exactly one assignment or select/parse the final runtime-effective value before asserting the JNI path.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
3 participants
What problem does this PR solve?
Backport #67158 to branch-4.1.
A clean parallel Paimon C++ build can start compiling the object library before bundled codec external projects install their headers. External Regression can also replace the packaged FE configuration without preserving the compatible ADBC JNI library path.
What is changed and how does it work?
Check List
Release note
None.