github-actions[bot] commented on code in PR #65503:
URL: https://github.com/apache/doris/pull/65503#discussion_r3567880112
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -371,14 +402,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) {
+ TableReader::set_batch_size(batch_size);
+ if (!_scanner_opened) {
+ return;
+ }
+ const auto status = _set_open_scanner_batch_size(_batch_size);
+ if (!status.ok()) {
Review 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 p
redictor lowers the batch.
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -116,10 +116,18 @@ 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";
+ // PAIMON_CPP is selected per split, but this scan-level selector cannot
inspect split-level
+ // table_format_params. Older FEs may also omit the scan-level
paimon_predicate marker. When the
+ // C++ reader option is enabled, conservatively keep JNI scans on V1 until
FileScannerV2 can
+ // dispatch PAIMON_CPP ranges itself.
+ const bool may_use_paimon_cpp_reader = scan_params.format_type ==
TFileFormatType::FORMAT_JNI &&
+
query_options.__isset.enable_paimon_cpp_reader &&
Review Comment:
This still leaves one rolling-upgrade Paimon JNI shape on V2. The selector
only disables V2 for `FORMAT_JNI` when `enable_paimon_cpp_reader` is true, but
the legacy scanner still treats missing `paimon_params.reader_type` as a
compatibility case and chooses `PaimonJniReader` when
`enable_paimon_cpp_reader=false`. V2 has no matching fallback:
`FileScannerV2::is_supported()` accepts Paimon JNI only when `reader_type ==
PAIMON_JNI`, and `PaimonHybridReader::_is_jni_split()` uses the same check. So
an old FE that sends `FORMAT_JNI` with `paimon_split`/predicate but no
`reader_type`, with V2 enabled and the C++ reader disabled, passes this guard
and then fails as unsupported in V2 instead of using legacy JNI. Please keep
possible legacy Paimon JNI ranges on V1 until the split-level reader type can
be inspected, or add a scan-level marker guaranteed for this old-FE case, and
cover the no-`reader_type`/`enable_paimon_cpp_reader=false` range.
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -329,11 +354,18 @@ 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());
- _reset_split_state(env);
- return Status::OK();
+ // 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);
+ }
+ if (cleanup_status.ok()) {
+ // Keep the Java object and opened state on failure so close() can
retry the cleanup.
+ _reset_split_state(env);
Review 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.
--
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]
