[fix](thirdparty) order Paimon codec dependencies - #67158
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review status: complete after 2 rounds. One blocking build-graph issue remains, so I am requesting changes.
Critical checkpoint conclusions
- Goal and proof: The PR correctly adds explicit object-library ordering for ZSTD, but it does not fully achieve the stated clean-parallel-build guarantee because the same object library directly requires LZ4 and still has no
lz4_eppredecessor. The focused test passes while that edge is absent. - Scope and clarity: The four-file change is small and focused. The workflow routing, patch application, and semantic-fingerprint update are otherwise coherent.
- Concurrency: The relevant concurrency is CMake/Ninja scheduling. In Doris's external-Arrow configuration,
paimon_objlibwaits forzstd_epandsnappy_epbut notlz4_ep; favorable scheduling or a host header can mask the race but is not an ordering guarantee. - Lifecycle: The ExternalProject configure/build/install lifecycle was traced through object compilation. Adding
lz4to this dependency fence is sufficient because the importedlz4target already ownsadd_dependencies(lz4 lz4_ep). No runtime or static-initialization lifecycle is involved. - Configuration and compatibility: No Doris runtime configuration is added. The current Paimon semantic hash matches the updated compatibility pin, only known complete legacy markers are admitted, artifact validation remains mandatory, and dependency ordering does not alter installed runtime artifacts.
- Parallel paths and conditions: Bundled Arrow is safe through
arrow_ep -> lz4_ep; Doris'sPAIMON_USE_EXTERNAL_ARROW=ONpath is the affected path. Static and shared outputs consume the same object library and have the same missing predecessor. Linux and macOS workflow routing is otherwise correct. - Tests and results: The Arrow/Paimon lifecycle test and the new focused script pass locally and in focused CI. The new script checks only
zstdandsnappyin a synthetic fragment, so it misses the LZ4 edge. Full Linux/macOS thirdparty builds and general compile/BE checks were still pending at submission; a passing run would not prove the missing graph edge because the race can be masked. - Error handling and observability: Patch/test failures are propagated with actionable diagnostics. No additional runtime logs or metrics are needed for this build-only change.
- Persistence, transactions, data correctness, memory accounting, and FE/BE protocol: Not applicable to this thirdparty build-graph change.
- Performance: No runtime performance impact was found; the dependency edges affect build scheduling only. No separate valuable performance issue survived review.
- Other issues: Two complete review rounds, including separate risk-focused scans, found no additional valuable finding. Every candidate was accepted, dismissed with evidence, or deduplicated.
User focus
No additional user-provided review focus was specified; the complete PR was reviewed.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review status: complete after 1 convergence round. Approval opinion: no blocking issue remains on the current head.
Critical checkpoint conclusions
- Goal and proof: The PR now adds
zstd,snappy, andlz4to the object-library dependency fence. In the exact patched Paimon external-Arrow/static-only configuration, generated Ninja and Unix Make graphs both place all three codec ExternalProject install targets before everypaimon_objlibobject compile. This resolves the earlier LZ4 thread; no distinct unordered codec-header path remains. - Scope and clarity: The four-file change is small and focused: one build-graph correction, the corresponding semantic-fingerprint migration, and focused CI coverage.
- Concurrency: The relevant concurrency is CMake/Ninja/Make scheduling. The imported codec targets transitively own
zstd_ep,snappy_ep, andlz4_ep, so their install stamps complete before object compilation while the independent codec builds can still run in parallel. No runtime shared-state concurrency is involved. - Lifecycle: The source-patch fingerprint forces stale source trees to be reset and repatched, component markers are invalidated before rebuild and republished only after required artifacts exist, and the ExternalProject install lifecycle now precedes header consumers. No runtime, ownership, or static-initialization lifecycle is changed.
- Configuration and compatibility: No Doris runtime configuration is added. The current Paimon semantic fingerprint computes exactly to
9887cf1ec13106da4250f8d7ea931a258879089c; legacy markers are admitted only at that selected-input fingerprint, artifact validation remains mandatory, and this ordering-only change does not alter installed archives or ABI, so no schema bump is required. - Parallel paths and conditions: External and internal Arrow paths, static-only and shared-capable object creation, direct and transitive codec-header users, Linux/macOS generators, workflow path filters, source rebuild, and prebuilt reuse were checked. The same object fence covers the affected paths.
- Tests and results:
bash -n,thirdparty/test/paimon-codec-dependency-test.sh,thirdparty/test/arrow-paimon-lifecycle-test.sh, exact-tag checksum verification, and full patch dry-run all pass locally. The live Arrow/Paimon Lifecycle Test and Thirdparty Script Test pass. The focused test is textual rather than graph-generating, but independent Ninja/Make graph inspection proves the current edge; full Linux and both macOS thirdparty builds plus general compile/BE checks were still running at submission. - Error handling and observability: Shell, patch, and fingerprint failures propagate with actionable messages. No runtime logs, metrics, or additional observability are warranted for this build-only change.
- Persistence, transactions, data correctness, memory accounting, and FE/BE protocol: Not applicable; no runtime data or protocol behavior is modified.
- Performance: No runtime performance impact was found. The added prerequisites only constrain the necessary build order and retain parallelism among independent codec builds.
- Other issues: Two complete full-coverage aspects and a separate risk-focused pass all returned
NO_NEW_VALUABLE_FINDINGS; the main review independently rechecked and closed every initial risk. No inline comment is warranted.
User focus
No additional user-provided review focus was specified; the complete PR was reviewed.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review status: complete after 1 convergence round on 4113dadea5fe0a8b72a3e49ab7cc5b8d71311a39. Approval opinion: no blocking issue remains on the current head.
Critical checkpoint conclusions
- Goal and proof: The Paimon change now places
zstd,snappy, andlz4on the object-library dependency fence. Configure-only inspection of the exact patchedpaimon-cpp-0a4f4e2Ninja graphs proves thatpaimon_objlibwaits forzstd_ep,snappy_ep, andlz4_epin both the Doris external-Arrow mode and the bundled-Arrow mode. This resolves the existing LZ4 thread; no distinct unordered generated-header path remains. - Scope and clarity: The codec ordering change, semantic-fingerprint migration, and focused CI coverage are small and coherent. The current head also includes a separate but narrow external-regression ADBC configuration parity repair with its own focused test; its packaging and startup behavior were independently traced and found correct.
- Concurrency: The relevant concurrency is clean parallel CMake/Ninja scheduling. Imported codec targets transitively own their ExternalProject install targets, so header installation completes before object compilation while the independent codec projects may still build concurrently. The new scripts use isolated temporary state. No runtime shared-memory concurrency or lock ordering is involved.
- Lifecycle: Source fingerprinting resets stale patch states, Paimon component markers are invalidated before rebuild and published only after required artifacts exist, and the codec install lifecycle now precedes all direct header consumers. For ADBC,
build.shcopieslibadbc_driver_jni.sotooutput/fe/lib, andstart_fe.shexports and expandsDORIS_HOMEbefore passing the JDK-17 options to Java. No ownership, cleanup, or static-initialization defect was found. - Configuration and compatibility: The ADBC item is a startup JVM system property, not a dynamic Doris configuration; restart semantics are appropriate. The Paimon semantic fingerprint computes exactly to
9887cf1ec13106da4250f8d7ea931a258879089c. Legacy reuse remains limited by that exact selected-input pin, exact historical markers, independent component markers, Arrow version validation, and complete required-library checks. The ordering-only delta changes no installed archive, ABI, protocol, or storage format. - Parallel paths and conditions: External and bundled Arrow, static-only and shared-capable object creation, codec target propagation, Linux/macOS generators, workflow path filters, source rebuild, prebuilt reuse, packaged versus external FE configuration, and ADBC library fallback behavior were checked. The affected parallel paths are covered.
- Tests and results: Local
thirdparty/test/paimon-codec-dependency-test.sh,thirdparty/test/adbc-jni-config-test.sh, andthirdparty/test/arrow-paimon-lifecycle-test.shall pass; shell syntax, workflow YAML parsing, and diff whitespace checks also pass. The live Arrow/Paimon Lifecycle Test and Thirdparty Script Test pass. The focused codec test is textual, but inspection of the real generated graphs provides the stronger ordering proof. Full Linux and both macOS thirdparty builds, plus general compile/BE/performance checks, were still running at submission. - Error handling and observability: Patch, fingerprint, configuration, and shell-test failures propagate with actionable diagnostics. Existing ADBC load errors identify the expected property/directory. No additional runtime metrics or logging are warranted for these build/configuration changes.
- Persistence, transactions, data correctness, memory accounting, and FE/BE protocol: Not applicable; this head changes build ordering, prebuilt selection metadata, CI checks, and an external test-deployment JVM option, not runtime data or protocol semantics.
- Performance and other issues: No runtime performance impact was found. The dependency fence constrains only the necessary build order and retains concurrency among independent codec builds. Two normal full-coverage reviews and a separate risk-focused review all returned
NO_NEW_VALUABLE_FINDINGS; the main review independently validated and closed every initial risk. No inline comment is warranted.
User focus
No additional user-provided review focus was specified; the complete six-file PR diff was reviewed.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4113dad to
e6f2cd0
Compare
|
run buildall |
|
/review |
|
skip buildall |
There was a problem hiding this comment.
Automated review status: complete after 2 convergence rounds. One P2 test-guard issue remains, so I am requesting changes.
Critical checkpoint conclusions
- Goal and proof: The Paimon change accomplishes its clean-parallel-build goal. In generated graphs from the exact patched
paimon-cpp-0a4f4e2source,paimon_objlibwaits forzstd_ep,snappy_ep, andlz4_epbefore object compilation. The added ADBC guard, however, does not always prove that the runtime-effective JDK-17 assignment retains the required JNI path; the inline comment covers that defect. - Scope and clarity: The codec ordering change, compatibility-marker migration, workflow wiring, external-FE comment, and two focused scripts are understandable and bounded. The remaining issue is isolated to the new ADBC guard.
- Concurrency: The affected concurrency is CMake/Ninja scheduling. The common object-library fence covers external/internal Arrow and static/shared outputs without a reverse edge or dependency cycle. No runtime shared-state concurrency, locks, or atomics are changed.
- Lifecycle: Codec ExternalProject install completion now precedes header consumers. Paimon markers are invalidated before rebuild and published only after install and required-artifact validation. For FE startup,
start_fe.shapplies active configuration assignments sequentially, which is why the guard must validate the effective final value. - Configuration and compatibility: The current external JDK-17 option expands correctly to the deployed FE
libdirectory and is a restart-time JVM property, not a dynamic Doris setting. The Paimon semantic fingerprint computes to9887cf1ec13106da4250f8d7ea931a258879089c; legacy reuse is restricted to that selected-input gate, enumerated markers, a matching Arrow component, and complete artifact closures. The ordering-only hunk changes no installed archive, ABI, protocol, or storage format. - Parallel paths and conditions: External and bundled Arrow, static/shared object reuse, codec target propagation, Linux/macOS workflow paths, source rebuild, prebuilt reuse, packaged versus external FE configuration, and the resolved LZ4 thread were checked. No additional distinct path issue survived review.
- Tests and results:
bash -nand all three focused scripts (arrow-paimon-lifecycle-test.sh,paimon-codec-dependency-test.sh, andadbc-jni-config-test.sh) pass locally; the live Arrow/Paimon Lifecycle Test and Thirdparty Script Test pass. A duplicate-assignment negative reproduction makes the ADBC guard pass while the runtime-effective value lacks the JNI override. Full Linux and both macOS thirdparty builds were still pending at submission; no builds were run in this review environment. - Error handling and observability: Patch, fingerprint, and script failures remain fail-fast with actionable diagnostics. No additional runtime logging or metrics are warranted for these build/configuration changes.
- Persistence, transactions, data correctness, memory accounting, FE/BE protocol, and security: Not applicable; this PR changes build ordering, prebuilt-selection metadata, CI guards, and an external deployment comment rather than runtime data or protocol behavior.
- Performance: No runtime cost was found. The dependency fence constrains only the necessary build order while independent codec projects may still run concurrently.
- Other issues: Round 2's two normal full-review tracks and separate risk-focused track all returned
NO_NEW_VALUABLE_FINDINGS. Every candidate is accepted, dismissed with evidence, or deduplicated; M-1 is the only inline comment.
User focus
No additional user-provided review focus was specified; the complete six-file PR diff was reviewed.
|
|
||
| # 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] Validate the runtime-effective JDK-17 assignment
grep returns every active JAVA_OPTS_FOR_JDK_17 assignment, so an earlier correct line can satisfy this check even after a later definition overrides it without the JNI option. start_fe.sh exports these assignments sequentially, making the later value effective; with the current line followed by JAVA_OPTS_FOR_JDK_17="-Xmx4096m", this script still reports PASS while FE lacks the override and can fall back to the Maven-bundled JNI library. Please validate the effective final assignment (or reject duplicates) and add that later-override negative case.
|
skip buildall |
|
Hi @Gabriel39 — I believe this change inadvertently breaks Paimon prebuilt reuse on the current CI build image, which is now failing the COMPILE and performance pipelines of PRs merged with master. Failure (before any compilation starts): Analysis:
Suggested fix: since the dependency-ordering change does not alter the installed artifacts (as the commit message notes), the old semantic marker should remain accepted — add Example failing builds on TeamCity: COMPILE 1034233, performance 1034228 (both on a PR merged with master after this change). Happy to send the one-line PR if that helps. Update (2026-08-29): the one-line fix is now up as #67305. Meanwhile, two manual retries of the COMPILE/performance pipelines on another PR (builds 1034257, 1034249, same image digest) failed at this same fingerprint check, so the issue is currently blocking all PRs merged with master. |
What problem does this PR solve?
A clean parallel Paimon C++ build can start compiling the object library before bundled codec external projects install their headers. This makes block_compression_factory.cpp fail with a missing codec header depending on the build graph state.
What is changed and how does it work?
Check List
Release note
None.