github-actions[bot] commented on code in PR #65503:
URL: https://github.com/apache/doris/pull/65503#discussion_r3567485087
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -329,11 +354,15 @@ Status JniTableReader::_close_jni_scanner() {
_collect_jni_scanner_profile(env);
// _fill_jni_block may fail before releasing the current Java table.
JniScanner::releaseTable()
- // is idempotent, so closing the split always releases it.
- RETURN_IF_ERROR(_jni_scanner_obj.call_void_method(env,
_jni_scanner_release_table).call());
- RETURN_IF_ERROR(_jni_scanner_obj.call_void_method(env,
_jni_scanner_close).call());
+ // is idempotent, so closing the split always releases it. Java close must
still run if that
+ // release fails; otherwise connector resources such as JDBC connections
can leak.
+ auto cleanup_status = _jni_scanner_obj.call_void_method(env,
_jni_scanner_release_table).call();
+ auto java_close_status = _jni_scanner_obj.call_void_method(env,
_jni_scanner_close).call();
Review Comment:
The retry path here is still not actually retryable. If `releaseTable()` or
Java `close()` returns a non-OK status, this line drops `_jni_scanner_obj` and
clears `_scanner_opened`; `JniTableReader::close()` then leaves `_closed`
false, but the next `close()` call enters `_close_jni_scanner()` with no Java
object and returns OK without invoking Java cleanup again. That matters for
scanners such as Hudi where `close()` can throw while the underlying reader
resource still exists, and more generally it makes a Java cleanup failure look
retryable when the retry has already been discarded. Please preserve the Java
object/open state until cleanup succeeds, or otherwise make the failure
terminal instead of pretending the next close can retry it. The new
`FailedCloseCanBeRetried` test should also use a faithful fake, because the
current override increments `close_calls` even after it has cleared the scanner
state.
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -371,14 +399,42 @@ Status JniTableReader::_open_jni_scanner() {
SCOPED_RAW_TIMER(&_jni_scanner_open_watcher);
RETURN_IF_ERROR(_register_jni_class_functions_once(env));
RETURN_IF_ERROR(_create_jni_scanner_object(env,
cast_set<int>(_batch_size)));
- // call open() method in JAVA side.
- RETURN_IF_ERROR(_jni_scanner_obj.call_void_method(env,
_jni_scanner_open).call());
- RETURN_ERROR_IF_EXC(env);
-
+ // Once the Java object exists, close it even if open() fails partway
through initialization.
+ // Connector implementations may already own streams, off-heap tables, or
JDBC connections.
_scanner_opened = true;
+ // call open() method in JAVA side.
+ const auto open_status = _jni_scanner_obj.call_void_method(env,
_jni_scanner_open).call();
+ if (!open_status.ok()) {
+ const auto close_status = _close_jni_scanner();
+ if (!close_status.ok()) {
+ LOG(WARNING) << "failed to clean up JNI scanner after open
failure: " << close_status;
+ }
Review Comment:
This still misses the first Paimon JNI physical batch. For a JNI split,
`prepare_split()` has already called `_open_jni_scanner()`, and
`_open_jni_scanner()` falls back to `RuntimeState::batch_size()` when
`_batch_size` is still zero. FileScannerV2 only initializes/predicts the
adaptive probe after that and then calls this method right before
`get_block()`. That updates `JniScanner.batchSize`, so Doris returns only the
probe rows, but Paimon already copied the constructor batch into
`CoreOptions.READ_BATCH_SIZE` during Java `open()` when it built the
table/reader; `setBatchSize()` does not rebuild that reader. The result is that
the first Paimon `reader.readBatch()` can still allocate/read the full runtime
batch, which defeats the adaptive memory cap this change is trying to provide.
Please seed the first adaptive size before eager JNI open, or update the Paimon
JNI reader's physical read batch before the first `readBatch()`, and add a
timing test for prepare -> adaptive probe ->
first JNI get.
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -116,10 +116,19 @@ bool
FileScanLocalState::_should_use_file_scanner_v2(const TQueryOptions& query_
const bool is_transactional_hive =
scan_params.__isset.table_format_params &&
scan_params.table_format_params.table_format_type ==
"transactional_hive";
+ const bool uses_paimon_cpp_reader =
Review Comment:
This check does not match the shape of real Paimon scan params.
`PaimonScanNode` sets the table-format descriptor on each `TFileRangeDesc` in
`setPaimonParams()`, while scan-level params only get values such as
`paimon_predicate` and backend options. So for a normal Paimon query with
`enable_file_scanner_v2=true` and `enable_paimon_cpp_reader=true`,
`scan_params.__isset.table_format_params` can still be false here and V2
remains selected. The first PAIMON_CPP `FORMAT_JNI` split then reaches V2 even
though `PaimonHybridReader` only dispatches `PAIMON_JNI` as JNI. Please key
this exclusion off information that is actually available at scan-selection
time, or have FE set an explicit scan-level marker, and add a test using
FE-shaped params with no scan-level `table_format_params`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
