Conversation
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.
sunchao
left a comment
There was a problem hiding this comment.
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_COMMITSfrom0to1, 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.
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 atformatVersion = 3(#6214), and on #6155 atformatVersion = 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.diffis regenerated from anapache-iceberg-1.9.1checkout with the existing diff and the two upstream commits applied:partial-progress.max-failed-commitsfrom 0 to 1 and renames the test totestParallelPartialProgressWithMaxCommitsLargerThanTotalGroupCount.Upstream changed every Spark version's copy of the test, so this PR changes both the
spark/v3.4andspark/v3.5copies in 1.9.1, although CI runs onlyv3.5for 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.1checkout, 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.