[fix](be) Harden JNI reader split lifecycle#65503
Open
Gabriel39 wants to merge 2 commits into
Open
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
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 30004 ms |
Contributor
TPC-DS: Total hot run time: 180273 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
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 && |
Contributor
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.
| return Status::OK(); | ||
| } | ||
| _closed = true; | ||
| RETURN_IF_ERROR(_close_jni_scanner()); |
Contributor
There was a problem hiding this comment.
This makes the JNI reader itself retryable, but the normal FileScannerV2 lifecycle still cannot reach the retry. FileScannerV2::close() calls _try_close() before _table_reader->close(), and _try_close() marks the scanner closed immediately. If Java releaseTable() or close() fails here, JniTableReader deliberately keeps _closed=false and preserves the Java object, but the next FileScannerV2::close() returns OK at the outer close guard without calling back into this reader. At that point destroying the reader only drops the JNI reference; Java cleanup is never retried. Please make the scanner-level close state commit only after table-reader cleanup succeeds, or track table-reader cleanup with a separate retryable flag, and cover the failing-then-succeeding close path through FileScannerV2.
Contributor
TPC-H: Total hot run time: 29530 ms |
Contributor
TPC-DS: Total hot run time: 181365 ms |
Contributor
ClickBench: Total hot run time: 25.09 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
This PR fixes all JNI-related To Do items under the DORIS-27038 epic:
Issue Number: DORIS-27053, DORIS-27058, DORIS-27059, DORIS-27064, DORIS-27077, DORIS-27078
Related PR: N/A
Release note
Harden JNI table-reader lifecycle handling and rolling-upgrade compatibility for Paimon and Hudi scans.
Check List
build-support/check-format.shandgit diff --check.stddef.hand pre-existing repository diagnostics.