Skip to content

[fix](paimon) recover JNI writes after OOM - #67114

Open
suxiaogang223 wants to merge 3 commits into
apache:branch-4.1from
suxiaogang223:codex/fix-paimon-jni-oom-recovery
Open

[fix](paimon) recover JNI writes after OOM#67114
suxiaogang223 wants to merge 3 commits into
apache:branch-4.1from
suxiaogang223:codex/fix-paimon-jni-oom-recovery

Conversation

@suxiaogang223

@suxiaogang223 suxiaogang223 commented Aug 25, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Problem Summary:

When a Paimon JNI writer hits an OOM while flushing or closing, Java cleanup can fail while asynchronous SDK tasks still reference Doris-owned native pages. The failed writer then cannot be safely released, and subsequent writes remain unavailable until the BE is restarted.

This PR:

  • makes Java resource cleanup retryable after Throwable, including OutOfMemoryError, and retains only resources whose close did not succeed;
  • avoids prepareCommit from the abort/OOM path because it can trigger more flushing and compaction allocation;
  • quarantines the Java writer together with PaimonJniMemoryManager when close cannot be confirmed;
  • retries quarantined close operations before admitting a later writer, releasing JNI ownership and native pages only after close succeeds;
  • keeps the JNI close state minimal: the writer global reference keeps its defining class loaded, so no separate class global reference is needed.

While cleanup still fails, new Paimon JNI writes fail fast instead of risking use-after-free. Once cleanup succeeds, later writes can proceed without restarting the BE.

Release note

Fix Paimon JNI writes remaining unavailable after an OOM during writer cleanup.

Check List (For Author)

  • Test
    • Regression test
    • Unit Test
    • Manual test
    • No need to test or manual test.
  • Behavior changed:
    • No.
    • Yes. Paimon JNI writer cleanup can be retried after OOM, allowing later writes to recover without a BE restart.
  • Does this need documentation?
    • No.
    • Yes.

Validation:

  • FE/BE-Java build: 27 modules passed, Checkstyle 0 violations.
  • PaimonJniWriterTest: 6 tests passed.
  • clang-format 16 dry-run and git diff --check passed.
  • BE C++ compilation and BE unit tests were intentionally not run for this change.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@suxiaogang223

Copy link
Copy Markdown
Member Author

run buildall

@suxiaogang223

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the proposed retryable cleanup still has four resource-lifecycle regressions.

Critical checkpoints:

  • Retry correctness and ownership: a failed Paimon writer is retained after its shared spill IOManager has been closed and its directories deleted, so later cleanup may never recover.
  • Abort/artifact cleanup: the all-failure no-prepare abort path loses already-flushed append/postpone file metadata and relies on a manual orphan-cleanup job that Doris does not run.
  • JNI lifecycle: the writer global reference, defining-class/method-ID lifetime, cross-thread JNIEnv acquisition, and native-manager release order are otherwise sound, but persistent retries leak exception-local JNI strings.
  • Arrow C Data: Arrow 19 allocator close is terminal-on-throw; a second no-op close can be mistaken for cleanup after a partial import retained foreign buffers.
  • Concurrency/authentication: mutex serialization, bounded in-flight admission, and retained authenticator behavior produced no additional issue.
  • Compatibility/configuration/performance: no separate regression found.
  • Tests: the new retryable lambda does not exercise real Paimon dependency ordering, real RootAllocator behavior, abort artifacts, or native quarantine repetition.
  • Release note: the intended recovery behavior is not yet achieved because the accepted retry paths can remain broken or falsely report success.
  • User focus: no additional focus was supplied.

Review status: complete. Round 2's two normal full-review subagents and separate risk-focused subagent all returned NO_NEW_VALUABLE_FINDINGS after falsifying the four accepted issues; no unresolved candidate remains.

Comment thread be/src/exec/sink/writer/paimon/jni_paimon_write_backend.cpp Outdated
@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 0.00% (0/82) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 74.14% (31169/42041)
Line Coverage 58.30% (345277/592212)
Region Coverage 55.01% (287205/522109)
Branch Coverage 55.99% (129514/231310)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 100% (0/0) 🎉
Increment coverage report
Complete coverage report

@Gabriel39 Gabriel39 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on making Paimon JNI cleanup recoverable after OOM. I found four lifecycle issues that need to be addressed before this recovery path is safe:

  1. Do not close the spill IOManager while the writer close is still unconfirmed (PaimonJniWriter.java:585-607). closeWriter() retains writer after writer.close() fails, but still closes ioManager. Paimon 1.4.2 closes its FileChannelManager by deleting the spill directories, so the retained writer can lose resources needed by its remaining flush/compaction tasks. A later retry may then fail permanently and keep Paimon writes fenced. Please retain the IOManager together with the writer and close it only after the writer has closed successfully.

  2. The new abort path can leave already-flushed files orphaned (PaimonJniWriter.java:671-691). On a write failure, preparedCommitMessages is normally empty, but append/postpone writers may already have flushed data files. In Paimon 1.4.2 those files are surfaced through prepareCommit; close alone does not delete all of them. Doris does not schedule remove_orphan_files, so relying on Paimon orphan cleanup can leak files indefinitely. Please preserve enough state to retry an explicit cleanup, or add a cleanup path that does not require a high-memory prepareCommit.

  3. RootAllocator.close() is not retryable in the way closeResources() assumes (PaimonJniWriter.java:531-549). Arrow 19.0.0 BaseAllocator sets isClosed before checking for outstanding buffers. If the first close throws, the second call returns immediately; this code then clears allocator and reports cleanup success even though imported C Data buffers may remain outstanding. Please track and close the imported resources themselves, and do not use a second allocator.close() return as proof that cleanup completed.

  4. Repeated native retries leak JNI local references (jni_paimon_write_backend.cpp:56-65). Each failed retry calls GetJniExceptionMsg with stack logging. That helper deletes the throwable but not the generated message and stack-trace jstrings. These calls run on long-lived attached BE threads without a local JNI frame, so repeated failures accumulate local refs and can trigger another OOM/local-reference-table exhaustion. Please delete those local refs or wrap each retry in PushLocalFrame/PopLocalFrame.

The compile and regular UT/regression checks pass, but BE changed-line coverage is 0/82 and the Arrow/Paimon lifecycle test is skipped, so these failure paths are currently untested.

@suxiaogang223

Copy link
Copy Markdown
Member Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 0.00% (0/89) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 59.41% (25642/43159)
Line Coverage 43.90% (262070/596903)
Region Coverage 39.82% (207501/521116)
Branch Coverage 41.32% (95742/231707)

@suxiaogang223

Copy link
Copy Markdown
Member Author

/review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants