github-actions[bot] commented on code in PR #65503:
URL: https://github.com/apache/doris/pull/65503#discussion_r3567487771
##########
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();
+ if (cleanup_status.ok() && !java_close_status.ok()) {
+ cleanup_status = std::move(java_close_status);
+ }
_reset_split_state(env);
Review Comment:
The retry path here is still not actually retryable. If `releaseTable()` or
Java `close()` returns a non-OK status, this reset 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;
+ }
+ return open_status;
+ }
return Status::OK();
}
+void JniTableReader::set_batch_size(size_t batch_size) {
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 `set_batch_size()` 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 pr
obe -> first JNI get.
##########
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.
--
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]
