[fix](be) Harden JNI reader split lifecycle#65503
Conversation
### What problem does this PR solve? JNI-backed table readers could violate scanner selection, cancellation, cleanup, EOF, adaptive batching, and rolling-upgrade contracts. Issue Number: N/A Related PR: N/A ### Release note Harden JNI table-reader lifecycle handling and compatibility for Paimon and Hudi scans. ### Check List - [x] Added unit tests for the fixes - [x] Added comments for compatibility and lifecycle invariants - [x] Passed 45 focused ASAN BE unit tests - [x] Passed repository format and diff checks
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
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 5:19 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
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 5:19 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 30004 ms |
TPC-DS: Total hot run time: 180273 ms |
ClickBench: Total hot run time: 24.99 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review found three blocking issues that should be fixed before merge:
- The PAIMON_CPP FileScannerV2 exclusion does not match real FE Paimon scan params, so PAIMON_CPP JNI splits can still be routed into V2.
- Failed JNI Java cleanup is made non-retryable even though
JniTableReader::close()leaves_closedfalse after failure. - Adaptive batch sizing reaches Paimon JNI after Java open, too late to cap Paimon's first physical
readBatch()size.
Checkpoint conclusions: the PR goal is only partially met because the lifecycle hardening and routing fixes still miss real execution paths. The change is focused, but the tests use simplified shapes that miss the FE-generated Paimon params, the real close-reset state transition, and Paimon's open-time batch option. No new lock-order or transaction/persistence issue was found. Mixed-version Paimon option fallback looks correct. Observability changes are not required, but M2 is a memory/performance correctness issue for the adaptive batch feature. Validation was static only, as the review prompt forbids builds and test execution.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65503 Problem Summary: Paimon C++ splits could still enter FileScannerV2 because scanner selection inspected a scan-level table-format descriptor that FE only sends per split. Failed JNI cleanup also discarded the Java object before a retry, and adaptive JNI sizing was applied after Paimon had already opened its physical reader. Use the scan-level Paimon predicate marker for routing, retain JNI state until cleanup succeeds, and seed adaptive batch sizing before split preparation opens JNI readers. ### Release note Ensure Paimon C++ scans use the supported scanner path, JNI cleanup remains retryable, and the first Paimon JNI batch honors adaptive sizing. ### Check List (For Author) - Test: Unit Test - Remote ASAN BE unit tests: FileScannerV2Test.PaimonCppReaderForcesLegacyScanner and JniTableReaderTest.* (6 tests passed) - Remote build-support/check-format.sh passed with clang-format 16 - Remote ASAN_UT changed objects compiled successfully - Remote clang-tidy analyzers completed for changed production files; final wrapper summary was not captured after the SSH output channel ended - Behavior changed: Yes, scanner routing, JNI cleanup retry state, and first-batch sizing are corrected - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review completed after 2 rounds. All five subagents converged on this final M1/M2 comment set with NO_NEW_VALUABLE_FINDINGS.
I found two blocking issues: one compatibility gap in the Paimon CPP V1/V2 selection, and one lifecycle gap where the new JNI close retry state cannot be reached through the scanner close path. I did not duplicate existing review threads for the prior first-Paimon-JNI-batch timing concern or the inner JNI close-state retryability thread.
Critical checkpoint conclusions:
- Goal: the PR addresses current-FE Paimon selection, JNI cleanup, cancellation, and adaptive batch feedback, but still misses rolling-upgrade Paimon CPP split-level metadata and scanner-level close retry.
- Scope: mostly focused, but the close retry fix crosses the
JniTableReader/FileScannerV2lifecycle boundary. - Concurrency: no new shared-thread synchronization was added; the issue is lifecycle state ordering during close.
- Lifecycle: not complete;
FileScannerV2marks itself closed before retained JNI cleanup state can be retried. - Configuration: no new config.
- Compatibility: not complete for old FE/new BE Paimon CPP shapes that keep Paimon metadata only at split level.
- Parallel paths: Paimon/Hudi hybrid batch forwarding was reviewed; no additional adaptive-batch issue found after the current seeding change.
- Tests: new BE tests are discovered by the existing CMake glob, but coverage is still missing for the accepted Paimon CPP rolling-upgrade selector and scanner-level failing-then-succeeding close path.
- Observability/performance: warning/profile changes and adaptive-batch seeding look reasonable; no separate metrics issue found.
- Transactions/persistence/data writes: not directly involved.
- User focus: no additional user-provided review focus.
Validation: static review only per the review prompt; no builds or unit tests were run. git diff --check is clean in this checkout.
| // FE stores Paimon's table-format descriptor per split, but paimon_predicate is a scan-level | ||
| // Paimon marker. PAIMON_CPP is also selected per split, and FileScannerV2 cannot dispatch that | ||
| // reader type, so retain the V1 path until the V2 hybrid reader supports it. | ||
| const bool uses_paimon_cpp_reader = scan_params.__isset.paimon_predicate && |
There was a problem hiding this comment.
The exclusion still misses the rolling-upgrade shape where Paimon metadata is only on each split. This selector only sees TFileScanRangeParams, so an old FE that has enable_paimon_cpp_reader=true but still sends Paimon predicate/reader metadata only in TFileRangeDesc.table_format_params.paimon_params will leave scan_params.__isset.paimon_predicate false and V2 remains selected. The first FORMAT_JNI range with TPaimonReaderType::PAIMON_CPP is not supported by V2: FileScannerV2::is_supported() accepts Paimon JNI only for PAIMON_JNI, and PaimonHybridReader dispatches only PAIMON_JNI to the JNI child. Please either use a marker that is guaranteed for both current and rolling-upgrade Paimon CPP scans, or conservatively keep JNI Paimon CPP on V1 until the split-level reader type can be inspected, and add a test for scan params without scan-level paimon_predicate plus a split-level PAIMON_CPP range.
TPC-H: Total hot run time: 29530 ms |
TPC-DS: Total hot run time: 181365 ms |
ClickBench: Total hot run time: 25.09 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65503 Problem Summary: FileScannerV2 selection could still route rolling-upgrade Paimon CPP JNI splits into an unsupported V2 reader, and a failed table-reader close left the scanner marked closed so retained JNI cleanup state could not be retried. Conservatively keep JNI scans on V1 while the Paimon C++ reader option is enabled, and roll back the scanner close guard when table-reader cleanup fails. ### Release note Harden Paimon CPP scanner selection and retry JNI cleanup after close failures. ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.* and JniTableReaderTest.*: 20 tests passed under ASAN BE UT - Behavior changed: Yes. Unsupported Paimon CPP JNI splits stay on V1, and failed V2 table-reader cleanup can be retried. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes based on static review.
Critical checkpoint conclusions:
- Goal: the PR addresses several prior JNI/FileScannerV2 review issues, but three compatibility/lifecycle/resource issues remain.
- Scope: the changes are focused on scanner selection, JNI cleanup, adaptive batch sizing, hybrid readers, and BE tests.
- Concurrency/lifecycle: scanner close retry and JNI resource lifecycle were reviewed; Paimon Java cleanup still invalidates retry state.
- Compatibility: rolling-upgrade Paimon JNI without reader_type is still not preserved on the V2 path.
- Parallel paths: FileScannerV1, FileScannerV2, Paimon/Hudi hybrid readers, and representative Java JNI scanners were checked.
- Testing: new BE tests cover useful cases, but they miss the three accepted edge cases below.
- Configuration/storage/transactions/observability: no new config, storage format, transaction, or observability requirement was found.
User focus: no additional user-provided focus was supplied.
Validation: static review only. I did not run builds or tests because the review prompt forbids build/test operations in this runner; the checkout also lacks .worktree_initialized and thirdparty/installed.
| return; | ||
| } | ||
| const auto status = _set_open_scanner_batch_size(_batch_size); | ||
| if (!status.ok()) { |
There was a problem hiding this comment.
Forwarding the later adaptive prediction to JniScanner.setBatchSize() still does not change Paimon's physical read batch after Java open(). FileScannerV2 updates the predictor after the first materialized block and calls set_batch_size() before the next get_block(), but this method only invokes Java setBatchSize, whose base implementation assigns batchSize. Paimon copied the constructor batch into CoreOptions.READ_BATCH_SIZE in buildTableOptions() while creating the reader during open(), and later reader.readBatch() calls keep using that existing RecordReader. The output loop stops appending after the new batchSize, but the connector can still read/materialize stale probe-sized batches, for example 32 very wide rows, on subsequent physical reads. Please make PaimonJniScanner.setBatchSize() update the physical reader batch, or keep Paimon JNI out of mid-split adaptive resizing, and add a test that observes the next physical readBatch() size after the predictor lowers the batch.
| } | ||
| if (cleanup_status.ok()) { | ||
| // Keep the Java object and opened state on failure so close() can retry the cleanup. | ||
| _reset_split_state(env); |
There was a problem hiding this comment.
Keeping the Java object on a failed close is not sufficient for Paimon, because PaimonJniScanner.close() clears the resources that would need the retry. releaseRecordIterator() nulls recordIterator in finally even if releaseBatch() throws; reader.close() and ioManager.close() also set their fields to null in finally before rethrowing. If any of those operations fails, C++ preserves _scanner_opened and will call close again, but the same Java object has already forgotten the iterator/reader/ioManager that may still be unclosed, so the retry is a no-op for that resource. Please only clear those Java fields after cleanup succeeds, or make such failures terminal rather than exposing them as retryable, and cover a Paimon-like close-fails-then-succeeds case.
### What problem does this PR solve? Issue Number: None Related PR: apache#65503 Problem Summary: Legacy Paimon JNI splits without reader_type could still enter FileScannerV2, adaptive batch-size updates after open did not resize Paimon physical readers, and failed Java cleanup cleared resource references before a retry. Keep JNI compatibility shapes on the legacy scanner, preserve the initial Paimon physical batch size after open, and retain each Java resource until its cleanup succeeds. ### Release note Harden legacy JNI selection, Paimon batch-size handling, and retryable Java cleanup. ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.*, JniTableReaderTest.*, and PaimonJniReaderTest.*: 30 tests passed under ASAN BE UT - PaimonJniScannerTest: 10 tests passed; checkstyle reported 0 violations - clang-format and git diff checks passed - Behavior changed: Yes. Legacy JNI compatibility splits stay on V1, Paimon ignores unsupported post-open physical batch resizing, and failed Java cleanup remains retryable. - Does this need documentation: No
2 participants
What problem does this PR solve?
JNI-backed file scans had several lifecycle, compatibility, and adaptive-batching gaps:
This PR hardens those paths by:
Release note
Harden JNI table-reader lifecycle handling, adaptive batching, and rolling-upgrade compatibility for Paimon and Hudi scans.
Check List
FileScannerV2Test.*andJniTableReaderTest.*after addressing review feedback.build-support/check-format.shandgit diff --check.stddef.hand pre-existing repository diagnostics.