Skip to content

branch-4.1: [fix](paimon) manage JNI writer lifecycle and spill - #66612

Merged
yiguolei merged 10 commits into
apache:branch-4.1from
suxiaogang223:codex/fix-paimon-jni-lifecycle-spill
Aug 27, 2026
Merged

branch-4.1: [fix](paimon) manage JNI writer lifecycle and spill#66612
yiguolei merged 10 commits into
apache:branch-4.1from
suxiaogang223:codex/fix-paimon-jni-lifecycle-spill

Conversation

@suxiaogang223

@suxiaogang223suxiaogang223 commented Aug 10, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Paimon JNI writers can create write-buffer, lookup, and clustering temporary files while background compaction tasks are still running. Previously, those files were not owned by Doris query spill lifecycle, and native callbacks or memory could be released before all Java tasks had stopped.

This PR fixes the lifecycle and spill-management issues:

  • Places each Paimon writer's temporary directory lazily under one Doris-managed, query-scoped spill root, following the same root selection, capacity check, usage accounting, and query cleanup model as internal spill.
  • Accounts Paimon buffer-channel writes through Doris spill callbacks and protects the query spill directory with a lease while asynchronous SDK work may still access it.
  • Stops and joins Paimon compaction work before releasing JNI resources, native memory, or spill callbacks; if task termination cannot be confirmed, dependent resources remain retained.
  • Uses scoped JNI references on writer open and error paths and avoids leaking native thread attachments.
  • Adds deterministic coverage for managed spill paths, cleanup retry, disabled-spill behavior, and writer thread lifecycle.

@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?

@suxiaogang223suxiaogang223 changed the title [fix](paimon) improve JNI writer resource lifecyclebranch-4.1: [fix](paimon) improve JNI writer resource lifecycleAug 10, 2026
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

1 similar comment
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage `` 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 0.00% (0/31) 🎉
Increment coverage report
Complete coverage report

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

/review

@suxiaogang223
suxiaogang223 marked this pull request as ready for review August 12, 2026 07:54

@github-actionsgithub-actionsBot 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.

Requesting changes: six correctness gaps remain in the new managed Paimon spill integration.

Critical checkpoints:

  • Native/JNI lifecycle: ScopedEnv attachment ownership, TLS invalidation, references, and signatures look correct. However, Paimon close cancellation does not join compaction before C++ releases the native spill callback.
  • Spill configuration and placement: write-buffer-spillable=false unnecessarily requires a usable spill root, while spillable writers are pinned to one disk without fallback.
  • Accounting and cleanup: lookup resets can leave deleted files charged, while lookup and clustering SST raw I/O bypasses capacity enforcement and metrics.
  • Regression coverage: the lifecycle test can pass the pre-fix bounded leak, and the added tests do not exercise these spill modes and failure paths.

No additional user focus was supplied; the whole 12-file change was reviewed. Three convergence rounds completed, and the final round reported no new valuable findings. This was a review-only task; builds and tests were not run per the runner instructions.

Comment threadbe/src/exec/sink/writer/paimon/jni_paimon_write_backend.cpp Outdated
Comment threadbe/src/exec/sink/writer/paimon/jni_paimon_write_backend.cpp Outdated
Comment threadbe/src/exec/spill/spill_file_manager.cpp Outdated
@suxiaogang223suxiaogang223 changed the title branch-4.1: [fix](paimon) improve JNI writer resource lifecyclebranch-4.1: [fix](paimon) manage JNI writer resources and spillAug 13, 2026
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage `` 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

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

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 34.44% (114/331) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage58.86% (25196/42805)
Line Coverage43.30% (256512/592455)
Region Coverage39.33% (203088/516385)
Branch Coverage40.82% (93672/229493)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 34.44% (114/331) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage73.83% (30799/41716)
Line Coverage57.96% (340790/588006)
Region Coverage54.79% (283666/517692)
Branch Coverage55.78% (127857/229223)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

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

@suxiaogang223
suxiaogang223force-pushed the codex/fix-paimon-jni-lifecycle-spill branch 3 times, most recently from 4e2184d to 5a3a6feCompareAugust 19, 2026 09:19
@suxiaogang223suxiaogang223 changed the title branch-4.1: [fix](paimon) manage JNI writer resources and spillbranch-4.1: [fix](paimon) manage JNI writer lifecycle and spillAug 19, 2026
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

/review

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z.
Workflow run: https://github.com/apache/doris/actions/runs/32239314646

Please trigger /review again after that time.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage `` 🎉
Increment coverage report
Complete coverage report

@suxiaogang223
suxiaogang223force-pushed the codex/fix-paimon-jni-lifecycle-spill branch from baa3a98 to b20dce8CompareAugust 21, 2026 06:30
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@suxiaogang223
suxiaogang223force-pushed the codex/fix-paimon-jni-lifecycle-spill branch from b20dce8 to a6e1b8fCompareAugust 24, 2026 08:39
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

1 similar comment
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

/review

@github-actionsgithub-actionsBot 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.

Requesting changes for three additional non-duplicate issues.

Correctness and accounting: a failed physical spill cleanup is logged and suppressed, after which native close releases the session accounting and query-directory lease while residual files still occupy the disk. Performance: every non-empty input block synchronously performs a mutex-held recursive stat of every managed spill tree. Regression coverage: the refreshed external-path assertion checks only allowed-root membership and no longer proves round-robin advancement.

I also traced JNI ABI/reference ownership, partial-open and abort cleanup, executor quiescence, lazy/no-root behavior, multi-root capacity, raw lookup/clustering and reset accounting, and the warmed thread-lifecycle oracle. Those remaining concerns are either covered by the six existing live inline threads or were disproved; I did not duplicate them. No additional user-provided review focus was supplied.

This static review used the authoritative bundled diff plus the exact Paimon 1.4.2 upstream paths. No builds or tests were run, per the review instructions. The final M2-M4 set converged in round 3 after round 2 removed an unsupported race candidate, and the live PR head/base and duplicate fence were reverified immediately before submission.

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

1 similar comment
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

/review

github-actions[bot]
github-actionsBot previously requested changes Aug 26, 2026

@github-actionsgithub-actionsBot 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.

Requesting changes for five newly substantiated issues on the bundled head: a reconciliation snapshot race that can double-charge callback-managed files; silent Iceberg data loss when spill-footer reservation fails; a global registry lock held across recursive deletion; reconciliation failures that can bypass abort for drained Paimon commit messages; and a post-quiescence accounting error that abandons prepared files and permanently fences later Paimon writers.

Critical checkpoints reviewed: native spill accounting and reservation rollback, multi-root/session ownership and query teardown, regular spill finalization/readback, Paimon executor quiescence, JNI partial-open/close cleanup, Java prepare/abort semantics, ABI/signature compatibility, memory-only lazy initialization, and the changed unit/regression oracles. Existing live threads and raw review comments were duplicate-fenced; no additional user focus was specified. The review is complete for head 61dbad4 after the required convergence round, subject to final API submission verification.

Comment threadbe/src/exec/spill/spill_file_manager.cpp Outdated
Comment threadbe/src/exec/spill/spill_file_writer.cpp Outdated
Comment threadbe/src/exec/spill/spill_file_manager.cpp Outdated
@suxiaogang223
suxiaogang223force-pushed the codex/fix-paimon-jni-lifecycle-spill branch from 61dbad4 to d65a910CompareAugust 26, 2026 09:41
@suxiaogang223

Copy link
Copy Markdown
MemberAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 36.61% (108/295) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage59.42% (25654/43171)
Line Coverage43.91% (262205/597081)
Region Coverage39.86% (207828/521330)
Branch Coverage41.33% (95797/231810)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 36.61% (108/295) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage74.24% (31229/42064)
Line Coverage58.45% (346385/592589)
Region Coverage55.24% (288692/522609)
Branch Coverage56.08% (129849/231544)

@yiguolei
yiguolei merged commit 5376423 into apache:branch-4.1Aug 27, 2026
30 of 35 checks passed
@github-actionsgithub-actionsBot added the approved Indicates a PR has been approved by one committer. label Aug 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@suxiaogang223
suxiaogang223 deleted the codex/fix-paimon-jni-lifecycle-spill branch August 28, 2026 07:01
suxiaogang223 added a commit to suxiaogang223/doris that referenced this pull request Sep 3, 2026
### What problem does this PR solve?
Issue Number: closeapache#65086
Related PR: apache#65868, apache#66612, apache#66810
Problem Summary: Add the BE Paimon sink, JNI writer backend, Java writer, commit payload transport, and worktree-local spill and memory lifecycle support required to write Paimon tables from master.
### Release note
Support writing Apache Paimon tables through the native Doris execution pipeline.
### Check List (For Author)
- Test: Static validation
- BE clang-format/check-format and FE checkstyle; full build and tests are deferred until all forward-port picks are complete
- Behavior changed: Yes, adds Paimon table writes
- Does this need documentation: Yes, documentation can follow separately
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approvedIndicates a PR has been approved by one committer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@suxiaogang223@hello-stephen@yiguolei@Gabriel39