Uh oh!
There was an error while loading. Please reload this page.
[test](paimon) Add P0 write correctness and snapshot reference coverage - #66325
Conversation
hello-stephen
commented
Jul 31, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 5th, 2026 4:09 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Requesting changes: the new regression coverage has seven P1 oracle and lifecycle issues that can let the intended bug escape detection, produce nondeterministic failures, or hang the regression runner.
Critical checkpoints:
- Correctness/oracles: the complex-value writer check reads the sink through both engines without comparing it to the OLAP source, and the snapshot-reference case does not make latest-generation sink rebinding observable.
- Concurrency/lifecycle: the JDBC helper neither proves commit overlap nor provides bounded all-path worker cleanup; the timeout stress worker can outlive the suite; and the thread-lifecycle test uses unscoped process-wide totals in the normal parallel pool.
- Memory/accounting: the stress test accepts unrelated failures, conflates controlled Doris allocation rejection with JVM heap exhaustion, and measures whole-JVM heap without enforcing isolation.
- Compatibility/configuration:
paimon.lock.enablednormalization, independent JDBC worker sessions, reference refresh/rejection behavior, and scalar source-model comparisons were traced and did not produce additional findings. - User focus: no additional review focus was supplied, so the full PR was reviewed.
Review verification: the review converged in the third and final round on head 1b6c9ef5de155b719bf6a1b8b55c06b552f778e7, with seven distinct inline comments and no unresolved candidates. This was a static review only; the review contract prohibited local builds or test execution. At final inspection, the live PR had no prior reviews or inline comments; several CI checks were successful, while compile and code-review checks were still pending.
| start.await() | ||
| rightAction() | ||
| } | ||
| assertTrue(ready.await(30, TimeUnit.SECONDS), |
There was a problem hiding this comment.
[P1] Bound and clean up both workers on every exit from this helper. If readiness times out, this assertion throws before start.countDown(), leaving a started action blocked; if left.get() throws, right.get() is skipped; and after dispatch both get() calls are unbounded, so a commit-lock deadlock can hang the runner. Release the latch in an unconditional path, wait for both workers with a deadline, and cancel/terminate both before rethrowing or starting catalog cleanup. Also add a commit-adjacent synchronization or observable in-flight check if these cases are meant to prove overlapping rather than merely simultaneous SQL dispatch.
| [(backendId): [ip.toString(), backendIdToHttpPort[backendId].toString()]] | ||
| } | ||
| assertFalse(backendEndpoints.isEmpty()) | ||
| def heapUsed = { |
There was a problem hiding this comment.
[P1] Run this heap oracle in an actually isolated suite. This case is tagged only p0,external,paimon, so when the opt-in flag is enabled it stays in the NORMAL pool (default suiteParallel=10) while sampling whole-JVM used heap on every BE. Allocations or GC from concurrent suites can exceed the 192 MiB allowance or inflate the baseline and mask a leak. Add nonConcurrent/dedicated invocation and scope measurements to the BEs/fragments that ran the sink or a writer-owned high-water metric.
| if (writerThread.isAlive()) { | ||
| // Cancel the stress query before failing so a timeout cannot leave its | ||
| // JDBC writer running after the regression suite has already finished. | ||
| activeStatement.get()?.cancel() |
There was a problem hiding this comment.
[P1] Keep controller-owned cleanup until this JDBC worker has actually stopped. cancel() may throw or may not terminate the query within ten seconds, and the connection is only owned inside this non-daemon thread; the following assertion can therefore fail while the worker/query remains alive and races catalog cleanup or hangs the runner. Retain and close the connection/statement from an unconditional cleanup path, preserving cancellation errors, and ensure the worker cannot outlive the suite.
| // A valid query may be rejected by a memory limit, but the embedded JVM | ||
| // must not be the component that exhausts memory outside Doris accounting. | ||
| assertFalse(failureMessage.contains("OutOfMemoryError"), |
There was a problem hiding this comment.
[P1] Reject unexpected writer failures instead of treating every non-OOM error as success. A catalog, connection, analysis, or unsupported-path failure exits early with low heap growth and passes this regression without exercising KEY_DYNAMIC memory. Require either a successful write with a data/snapshot oracle or one specifically whitelisted Doris memory-limit outcome, and distinguish Java heap exhaustion from the tracked native-page error by message/cause rather than the exception class name alone.
| ORDER BY id | ||
| """)) | ||
| // A historical source relation must keep its own schema/snapshot while |
There was a problem hiding this comment.
[P1] Make the latest sink generation observably different from the historical source. The baseline and current table keep the same four-column schema, so a sink accidentally reused from the historical FileStoreTable can still append these rows and satisfy every assertion. Evolve the main schema after creating the refs (for example, add a column with a default), then assert both the old reference schema and the latest sink/default behavior.
| from paimon.${dbName}.complex_sink | ||
| order by id | ||
| """ | ||
| assertSparkDorisResultEquals(sparkComplexRows, complexRows) |
There was a problem hiding this comment.
[P1] Compare the complex sink with the original OLAP values. Both result sets here read complex_sink, so a writer-side conversion bug can persist the same corrupted value that Spark and Doris later agree on. Capture the internal source rows or use explicit expected values, and compare those against the sink in addition to this cross-reader check.
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| suite("test_paimon_write_thread_lifecycle", "p0,external,paimon") { |
There was a problem hiding this comment.
[P1] Do not run this process-wide thread-count oracle alongside normal parallel suites. This tag set is assigned to the NORMAL pool (default suiteParallel=10), while both sampled metrics include all BE/JVM threads, so unrelated suites can make a clean writer fail or mask a leak. At minimum isolate it with nonConcurrent; preferably poll a Paimon-specific thread prefix or lifecycle gauge on the BEs that executed the sinks.
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
There was a problem hiding this comment.
Requesting changes on head 85847c74378a1a7dd2707eec3a4a95288afba523: the latest import fix correctly resolves the Http helper reference, but it does not address the seven existing P1 inline threads, all of which remain applicable and are already mapped to the current head. Duplicate suppression therefore leaves this review with no new inline comments.
Critical checkpoints:
- Correctness/oracles: the historical-source case still cannot distinguish a stale historical
FileStoreTablefrom the latest writable generation, and the recursive complex-value check still compares two reads of the sink rather than the OLAP source values. - Concurrency/lifecycle: the JDBC barrier proves only SQL dispatch, not overlapping commits, and lacks bounded all-path worker cleanup; the timed stress worker can outlive controller cleanup; and the lifecycle suite still uses unscoped process-wide thread totals in the normal parallel pool.
- Memory/accounting: the KEY_DYNAMIC test still samples unisolated whole-JVM heap, accepts unrelated writer failures, and can also interpret registered-but-invalid zero JVM gauges as zero growth. This is additional evidence for the existing heap-oracle thread, whose writer-owned high-water recommendation covers the same defect.
- Configuration/compatibility: JDBC lock-option normalization, independent regression-thread JDBC sessions, historical tag/branch pinning, rejected branch-sink non-mutation, scalar OLAP source-model comparisons, and the new
Httputility import were traced without another distinct finding. - Security/scope: these are test-only changes and introduce no new production authentication or input-handling surface.
- User focus: no additional review focus was supplied, so the whole PR was reviewed.
Review completion: the third and final round converged on the frozen head, ledger, existing-thread set, and this summary with no unresolved candidates and no new distinct inline findings. All seven substantiated issues are already represented by the existing threads. This was a static review only; the review contract prohibited local builds and test execution.
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
There was a problem hiding this comment.
Requesting changes on head 8837b52a6d8c0a694e32d4692af750d9e1ea71e5: one additional distinct P1 oracle gap survives this review, and the seven existing P1 inline threads remain applicable.
Critical checkpoints:
- Correctness/oracles: the historical snapshot and branch copies transform DECIMAL values and carry a microsecond timestamp, but the current-table assertions check only ID/payload and the later full-row check compares two readers of the same sink. The existing latest-generation sink-binding and complex OLAP-source oracle gaps also remain open.
- Concurrency/lifecycle: the JDBC barrier still neither proves overlapping commits nor provides bounded all-path worker cleanup; the timed KEY_DYNAMIC worker can outlive controller cleanup; and the thread reproducer still uses unscoped process-wide totals in the normal parallel pool.
- Memory/accounting: the KEY_DYNAMIC test still samples unisolated whole-JVM heap and accepts unrelated writer failures, so it can pass without exercising the intended memory path.
- Configuration/compatibility: JDBC lock-option normalization, independent regression-thread JDBC sessions, historical tag/branch pinning, pre-execution rejection of unsupported branch sinks, scalar OLAP source-model comparisons, and result normalization were traced without another distinct finding.
- Security/scope: these are test-only changes and introduce no new production authentication or input-handling surface.
- User focus: no additional review focus was supplied, so the entire PR was reviewed.
Review completion: the third and final convergence round rechecked all five changed files against this exact payload, the frozen ledger, and the live existing-thread set with no unresolved candidates or additional valuable findings. This was a static review only; the review contract prohibited local builds or test execution. No required AGENTS.md files existed for the changed paths, and the prompt-required repository review skill was absent from both the checkout and HEAD, so the complete bundle contract was used as the available review procedure.
| FROM ${tableName} FOR VERSION AS OF ${baselineSnapshot} | ||
| """ | ||
| sql """refresh table ${tableName}""" | ||
| assertEquals([[1, "base"], [2, "latest"], [101, "base-snapshot-copy"]], sql(""" |
There was a problem hiding this comment.
[P1] Check every value copied from the historical relations, not just the ID and payload. These INSERT SELECTs also transform amount and carry the microsecond timestamp, but the only later query that reads those columns compares Spark and Doris views of the same Paimon sink. A writer-side decimal/timestamp corruption can therefore persist identically for both readers and still pass. Assert independent full-row expectations for the snapshot and branch copies (including 11.25/12.25 and the original timestamp). This is a separate path from the existing complex_sink oracle thread, which covers nested OLAP-source values.
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: closeapache#65086 Related PR: apache#65868, apache#66325, apache#67023, apache#67085 Problem Summary: Forward-port the final branch-4.1 Paimon write regression matrix to master, including append and primary-key modes, bucket routing, transactions, schema evolution, row-level DML, Variant V2, snapshot references, writer lifecycle, cache coherence, and stabilized external paths. Generated golden outputs are preserved byte-for-byte from branch-4.1. ### Release note None ### Check List (For Author) - Test: Regression test - Test cases and generated expected outputs were forward-ported; execution is deferred until the final integrated validation. - Behavior changed: No - Does this need documentation: No
Proposed changes
Motivation
The existing P0 suites cover broad serial write behavior, table models, merge engines, scalar and nested types, schema evolution, transactions, and failure paths. They did not exercise independent Doris transactions committing concurrently to one Paimon table, verify time travel and references rooted in snapshots written by Doris, or validate every Doris OLAP source model and recursive complex values through INSERT SELECT.
Validation