Skip to content

fix: close Iceberg cleanup ownership gap during JVM handoff - #6582

Merged
andygrove merged 1 commit into
apache:mainfrom
unikdahal:fix/iceberg-write-cleanup-handoff
Oct 4, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
unikdahal:fix/iceberg-write-cleanup-handoff

Conversation

@unikdahal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #6581

Rationale for this change

Close the remaining cleanup ownership gap during the native → JVM Iceberg write handoff.

What changes are included in this PR?

  • Keep AbortOnDrop armed until the JVM polls the native stream to EOF.
  • Add a pre-handoff failure-injection test.
  • Update cleanup ownership documentation.

How are these changes tested?

  • Rust coverage for acknowledged vs unacknowledged handoff.
  • End-to-end Iceberg failure injection verifies no commit and no orphan data files.
  • Existing Iceberg write CI matrix.

@github-actions github-actions Bot added bug Something isn't working area:writer Native Parquet writer area:Iceberg labels Oct 3, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

  • Prior state and problem: Native cleanup was disarmed before the JVM recorded the written-file locations. A failure during that handoff could leave orphan files.
  • Design approach: output_with_cleanup_ack retains AbortOnDrop until the JVM polls EOF after taking cleanup ownership.
  • Correctness / compatibility analysis: Traced the stream through JNI and CometExecIterator. The writer uses synchronous JVM-fed polling, and locations are registered before EOF. Checked Spark 3.4–4.2 task-listener semantics and the four pinned Iceberg versions’ abort implementations. No introduced P1/P2 issues found within this review.
  • Key design decisions: The brief ownership overlap closes the gap while preserving best-effort cleanup. Acknowledgment stays within the existing stream protocol. Successful writes retain the existing JNI call count and add one failpoint lookup per task.
  • Implementation sketch: Adds the guarded output stream, a scoped pre-handoff failpoint, Rust and Scala regression coverage, and matching ownership documentation.
  • Behavioral changes worth calling out: Failed handoffs now delete uncommitted files. Successful writes preserve their files and results. The same early-disarm gap exists on branch-1.1, so this is an intended cleanup improvement over that release.
  • Suggested improvements: None meeting the requested P1/P2 threshold.

Reviewed full SHA 738a0319bda833ecfe6189afeba03a3d631abe56 against base 569eaa59d032f758669777964aee2d24eb55ebae. Reviewed the complete six-file PR diff and reconciled the broader tree comparison: the two commits present only on the base are not PR regressions. No stacked prerequisites were omitted. The PR remains non-draft, with no existing reviews, comments, or review threads.

Routed skills: review-comet-pr, review-comet-iceberg-write-pr, and review-comet-ffi-pr. Expression and shuffle skills were also consulted while checking the base-only differences.

Exact-head CI: the label workflow passed. Comet CI, CodeQL, and Check PR Title remain action_required, without build/test verdicts.

Validation: native build and Scala test compilation passed. Three Rust lifecycle tests passed. Three Scala tests passed on Spark 4.1.3 / Iceberg 1.11.0 / JDK 17: pre-handoff failure cleanup, post-handoff failure cleanup, and successful native append. git diff --check passed. Native validation disabled optional HDFS support, and formatting checks were skipped. Full Spark SQL/Iceberg suites, other runtime versions, and real object stores were not exercised.

@unikdahal unikdahal mentioned this pull request Oct 3, 2026
2 of 8 tasks
@andygrove
andygrove enabled auto-merge October 4, 2026 21:14
@andygrove
andygrove added this pull request to the merge queue Oct 4, 2026
Merged via the queue into apache:main with commit 216c938 Oct 4, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Iceberg area:writer Native Parquet writer bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native Iceberg Write: close cleanup ownership gap during JVM handoff

3 participants