github-actions[bot] commented on code in PR #67114:
URL: https://github.com/apache/doris/pull/67114#discussion_r3852019683
##########
be/src/exec/sink/writer/paimon/jni_paimon_write_backend.cpp:
##########
@@ -46,31 +46,86 @@ namespace doris {
namespace {
constexpr std::string_view PAIMON_JNI_WRITER_IO_TMP_DIR =
"paimon_jni_writer_io_tmp";
-std::atomic<bool>& paimon_jni_close_failed() {
- static std::atomic<bool> failed {false};
- return failed;
-}
+struct PaimonJniCloseState {
+ // The global writer reference keeps both the Java object and its defining
class alive. The
+ // cached method ID therefore remains valid until release_writer_ref()
deletes this reference.
+ jobject writer_obj = nullptr;
+ jmethodID close_id = nullptr;
+ std::unique_ptr<PaimonJniMemoryManager> memory_manager;
+
+ Status retry_close(JNIEnv* env) const {
+ if (writer_obj == nullptr) {
+ return Status::OK();
+ }
+ if (close_id == nullptr) {
+ return Status::InternalError("PaimonJniWriter.close method is
unavailable");
+ }
+ env->CallVoidMethod(writer_obj, close_id);
+ return Jni::Env::GetJniExceptionMsg(env, true,
Review Comment:
[P2] Bound JNI locals across repeated close failures.
This call is reached on every later writer open while the quarantined close
keeps failing. `GetJniExceptionMsg` creates `msg` and, with `log_stack=true`,
`stack` local jstrings but deletes only the original throwable. These opens run
on attached long-lived native workers with a TLS `JNIEnv`, so there is no
Java-to-native return or local frame that reclaims those references; they
accumulate on every retry. The old atomic fence avoided all later JNI calls.
Please bracket the retry with `PushLocalFrame`/`PopLocalFrame` or release both
temporaries in the helper.
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonJniWriter.java:
##########
@@ -265,8 +264,12 @@ public byte[][] prepareCommit() throws Exception {
}
/**
- * Abort: discard all written data files and close the SDK writer.
+ * Abort an already prepared commit, if present, and close the SDK writer.
* Called from C++ when write or prepareCommit fails.
+ *
+ * <p>Do not call prepareCommit from this error path: preparing may flush
and compact data,
+ * which needs more heap precisely when the caller is recovering from an
OOM. Files without a
+ * prepared commit message remain uncommitted and are handled by Paimon's
orphan cleanup.
Review Comment:
[P1] Preserve cleanup for non-OOM aborts.
C++ calls the same `abort()` for every write or prepare failure, so this
no-prepare rule is not limited to OOM. Paimon append-only and postpone writers
can already have completed files in their pending `newFiles` lists after an
intermediate flush; `close()` does not delete them. Previously `prepareCommit`
exposed those files to `InnerTableCommit.abort` for deletion. Now the
empty-message path loses their metadata, and Paimon orphan removal is a
separately submitted maintenance job that Doris never runs. Avoid prepare only
for the OOM case or add a no-flush path that explicitly deletes already
materialized files.
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonJniWriter.java:
##########
@@ -526,16 +529,24 @@ private void trackFullCompactionBucket(SinkRecord
sinkRecord) {
// ────────────────────────────────────────────────────────────
private void closeResources() throws Exception {
+ Throwable failure = null;
try {
closeWriter();
- } finally {
- writeSchema = null;
- arrowAdapter = null;
- if (allocator != null) {
- allocator.close();
+ } catch (Throwable closeFailure) {
+ failure = closeFailure;
+ }
+
+ writeSchema = null;
+ arrowAdapter = null;
+ if (allocator != null) {
+ Throwable allocatorFailure = closeResource(allocator);
+ if (allocatorFailure == null) {
Review Comment:
[P1] Do not treat a second Arrow allocator close as successful recovery.
In pinned Arrow 19, `BaseAllocator.close` sets `isClosed` before checking
outstanding buffers, so after the first close throws the next call returns
immediately. A partial `Data.importVectorSchemaRoot` can create and partly load
a root, then throw before the initializer returns; this try-with-resources
never acquires `root` and cannot close its foreign buffers. This branch retains
the allocator on the real failure, then nulls it after the no-op retry,
allowing C++ to remove the admission fence while moved C Data resources remain.
Handle partial imports or the allocator terminal state explicitly, and test
with a real `RootAllocator`.
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonJniWriter.java:
##########
@@ -572,35 +583,76 @@ private void submitFullCompaction() throws Exception {
}
private void closeWriter() throws Exception {
- if (sdkCloseFailed) {
- throw new IllegalStateException(
- "A previous Paimon SDK close failed; native memory cannot
be released safely");
+ Throwable failure = null;
+
+ Throwable writerFailure = closeResource(writer);
+ if (writerFailure == null) {
+ writer = null;
+ } else {
+ failure = writerFailure;
+ }
+
+ Throwable indexAssignerFailure = closeResource(globalIndexAssigner);
+ if (indexAssignerFailure == null) {
+ globalIndexAssigner = null;
+ } else {
+ failure = mergeFailure(failure, indexAssignerFailure);
}
- Exception failure = closeResource(writer, null);
- failure = closeResource(globalIndexAssigner, failure);
- failure = closeResource(ioManager, failure);
- clearWriterState();
- if (failure != null) {
- sdkCloseFailed = true;
- throw failure;
+
+ Throwable ioManagerFailure = closeResource(ioManager);
Review Comment:
[P1] Keep the IOManager until the retained writer has closed.
In Paimon 1.4.2, `AbstractFileStoreWrite.close` exits on the first
bucket-writer exception and does not clear the remaining writers. This code
retains that writer graph, but still calls `IOManager.close`, whose
implementation deletes the shared spill directories. A later quarantine retry
therefore re-enters live bucket writers after their spill backing was removed,
turning a transient OOM/close failure into a permanent one. Only close and null
the IOManager after `writer.close` succeeds, and cover a multi-bucket partial
close.
--
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]
