Skip to content

test: backport the upstream fix for a flaky Iceberg 1.9.1 rewrite test - #6242

Queued
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:iceberg-191-rewrite-flake
Queued

andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:iceberg-191-rewrite-flake

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

No open issue. This follows up on #6214, where the flake was first diagnosed.

Rationale for this change

The Iceberg 1.9 shard-2 job has failed twice in two days on the same test, TestRewriteDataFilesAction.testParallelPartialProgressWithMaxFailedCommitsLargerThanTotalFileGroup: in the 2026-09-25 nightly at formatVersion = 3 (#6214), and on #6155 at formatVersion = 2. Both runs failed with "1 rewrite commits failed. This is more than the maximum allowed failures of 0", and both passed on a re-run.

This is a known upstream flake, apache/iceberg#12889. The test has three threads commit ten file groups one at a time against a Hadoop table. A commit that keeps losing the race runs out of Iceberg's default four commit retries, and because the test allows no failed commits, that one starved commit fails it. None of this involves Comet code. Upstream fixed the test in apache/iceberg#13208 and apache/iceberg#13598. Both fixes are in 1.10.0 but not 1.9.1, which is why the 1.10 shard-2 job keeps passing.

The Iceberg 1.9 job runs only in the nightly and on pull requests labeled run-iceberg-tests, so each failure either turns a nightly red or costs a labeled pull request a re-run.

What changes are included in this PR?

dev/diffs/iceberg/1.9.1.diff is regenerated from an apache-iceberg-1.9.1 checkout with the existing diff and the two upstream commits applied:

Upstream changed every Spark version's copy of the test, so this PR changes both the spark/v3.4 and spark/v3.5 copies in 1.9.1, although CI runs only v3.5 for 1.9.1. The resulting method is identical to the one in 1.10.0. The diff gains those two file sections and nothing else. Before making the edit, I checked that regenerating the unmodified diff reproduced the committed file exactly.

How are these changes tested?

The regenerated diff applies cleanly to a fresh apache-iceberg-1.9.1 checkout, reproduces itself when regenerated from there, and leaves the same tree as the edited clone. I applied the upstream commits as three-way merges rather than as patches, because the line the second commit replaces, shouldHaveSnapshots(table, 11);, appears twice in the file and an offset patch could land on the wrong test.

Iceberg's test sources can't be built locally here, so this PR carries run-iceberg-tests, which compiles the patched Iceberg and runs the test in the 1.9 shard-2 job. One green run can't show that a flake is gone. The case for the fix is upstream's, and the 1.10 shard-2 job, which runs the fixed test, has passed in every nightly since 2026-09-16.

TestRewriteDataFilesAction.testParallelPartialProgressWithMaxFailedCommitsLargerThanTotalFileGroup
fails now and then in the Iceberg 1.9 shard-2 job with "1 rewrite commits failed. This is more than
the maximum allowed failures of 0". Three threads commit ten file groups one at a time against a
Hadoop table, and a commit that keeps losing the race runs out of Iceberg's default four commit
retries. The test allows no failed commits, so that one starved commit fails it. This is
apache/iceberg#12889, and it happens in Iceberg's commit code, not Comet's.

Upstream fixed the test in apache/iceberg#13208, which allows one failed commit, and
apache/iceberg#13598, which then expects at least 10 snapshots instead of exactly 11. Both are in
1.10.0 but not 1.9.1. Regenerate the 1.9.1 diff with both applied to the spark/v3.4 and spark/v3.5
copies of the test, as upstream applied them to every Spark version. The method now matches
1.10.0's.
@github-actions github-actions Bot added enhancement New feature or request test Testing related area:Iceberg labels Sep 26, 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: Iceberg 1.9.1’s parallel rewrite test allowed no failed commits, making concurrent Hadoop-table commit contention fail the test intermittently.
  • Design approach: Backport upstream apache/iceberg#13208 and #13598 into the Spark 3.4 and 3.5 test copies.
  • Correctness / compatibility analysis: Both patched methods exactly match Iceberg 1.10.0. Iceberg 1.9.1’s commit accounting supports the revised expectation: nine successful rewrites plus the initial snapshot yields ten snapshots. Row equality, orphan-file and cache assertions remain, as does the neighboring zero-failure test. Checked the corresponding Iceberg implementations and Spark 3.4.3/3.5.9 commit/abort sources.
  • Key design decisions: The adjustment stays confined to one test in each source set. It introduces no production runtime overhead, new abstraction or configuration default change.
  • Implementation sketch: Rename the test, change PARTIAL_PROGRESS_MAX_FAILED_COMMITS from 0 to 1, and replace the exact eleven-snapshot assertion with a minimum of ten.
  • Behavioral changes worth calling out: This test now tolerates one unsuccessful rewrite commit while still checking data preservation and cleanup.
  • Suggested improvements: None at P1/P2. No introduced P1/P2 issues found within this review.

Reviewed the entire diff from 14f0f59f74dbcf273ff3cbdb1721a5ee535bbe84 to 944bbcf8e5874bed8454dfa5fe605e81ef66c753. The PR is not a draft. The supplied snapshot and live discussion contain no existing reviews, comments or threads.

Routed skills: review-comet-pr and review-comet-iceberg-write-pr.

Exact-head CI: Comet CI passed all Iceberg 1.9 jobs, including shard 2. Its downloaded JUnit report records all 104 TestRewriteDataFilesAction cases passing, with 52 each for format versions 2 and 3 and no skips. Iceberg 1.11 extensions remained running at the final check. The separate label run was cancelled during setup, causing its aggregate check to fail before tests ran.

Validation: Applied the complete patch to a fresh apache-iceberg-1.9.1 checkout and regenerated it byte-for-byte. Verified every pre-existing patch section is unchanged and both edited methods match upstream. No local JVM/native build or repeated stress run was performed. The Spark 3.4 copy was validated through source comparison and patch application. A single passing CI run cannot establish that the flake is eliminated.

@andygrove
andygrove added this pull request to the merge queue Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Iceberg enhancement New feature or request run-iceberg-tests test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants