istreeter commented on PR #18407:
URL: https://github.com/apache/iceberg/pull/18407#issuecomment-6425764768
This PR is closely related to an issue I've been looking at while working on
commit retries in an application that has several concurrent writers committing
to the same Iceberg table. It's a second consequence of the same "applied, then
409 on retry" condition as #18402. This PR doesn't fix it, but the
`stagedNewSnapshotId` it adds is exactly what's needed to.
With concurrent writers, the retry can succeed by moving the branch
**backwards**:
1. Writer 1 commits snapshot X. The catalog applies it, but writer 1 sees a
`CommitFailedException`.
2. Writer 2 commits Y on top of X. `main` is now Y.
3. Writer 1 retries. `apply()` rebuilds on top of Y but reuses the cached id
X, and since `base.snapshot(X) != null` it takes the `// this is a rollback
operation` branch: it discards the rebuilt snapshot and points `main` back at X
by id. Unlike a real rollback, nothing has checked whether moving `main` back
past Y is intended, and the catalog's concurrency check passes because writer
1's view of `main` is current.
4. `main` is X again and Y is orphaned. Both `commit()` calls return
normally. Writer 2's data is gone from the live table, and once expiry removes
Y it's gone for good.
A minimal test in the style of the one in this PR (for
`TestSnapshotProducer`). On `main` it fails for all format versions with
`expected: 2L but was: 1L`. In other words, writer 1's commit returns normally
and `main` has been rewound to X.
```java
@TestTemplate
public void retryAfterAppliedCommitKeepsConcurrentSnapshot() {
// Simulates a catalog that applies a commit but reports it as failed,
with another writer
// committing on top of it before the retry.
String tableName = "apply-then-concurrent-commit";
AtomicLong concurrentSnapshotId = new AtomicLong(-1L);
TestTables.TestTableOperations ops =
new TestTables.TestTableOperations(tableName, tableDir) {
private boolean firstCommit = true;
@Override
public void commit(TableMetadata base, TableMetadata
updatedMetadata) {
super.commit(base, updatedMetadata);
if (base != null && firstCommit) {
this.firstCommit = false;
Table concurrent = TestTables.load(tableDir, tableName);
concurrent.newFastAppend().appendFile(FILE_B).commit();
concurrentSnapshotId.set(concurrent.currentSnapshot().snapshotId());
throw new CommitFailedException("Injected
apply-but-report-failure");
}
}
};
TestTables.TestTable commitTable =
TestTables.create(
tableDir, tableName, SCHEMA, SPEC, SortOrder.unsorted(),
formatVersion, ops);
commitTable.newFastAppend().appendFile(FILE_A).commit();
assertThat(commitTable.currentSnapshot().snapshotId())
.as("The concurrent writer's snapshot must remain the head of main")
.isEqualTo(concurrentSnapshotId.get());
}
```
This PR doesn't touch the rollback branch, and the commit succeeds, so the
new failure-path cleanup never runs. But the `stagedNewSnapshotId` this PR adds
is the missing piece. @pvary suggested earlier "only throw an exception if the
commit is not already there on the history", and that idea fixes this too: if
the snapshot is one this `commit()` call staged on an earlier attempt, the
earlier attempt was applied and there's nothing left to change. On top of this
branch, in `SnapshotProducer.commit()`:
```diff
--- a/core/src/main/java/org/apache/iceberg/SnapshotProducer.java
+++ b/core/src/main/java/org/apache/iceberg/SnapshotProducer.java
@@ -509,4 +509,8 @@ public void commit() {
if (base.snapshot(newSnapshot.snapshotId()) != null) {
- // this is a rollback operation
- update.setBranchSnapshot(newSnapshot.snapshotId(),
targetBranch);
+ // a snapshot staged as new by an earlier attempt of
this call was applied, and
+ // the branch may have moved on since, so there is
nothing left to change
+ if (newSnapshot.snapshotId() !=
stagedNewSnapshotId.get()) {
+ // this is a rollback operation
+ update.setBranchSnapshot(newSnapshot.snapshotId(),
targetBranch);
+ }
} else {
```
With no changes, the existing `updated.changes().isEmpty()` check skips the
commit, and the success path keeps the snapshot's files. On top of this branch
(`94bb0d307`), the test above passes, and so do `TestSnapshotProducer`,
`TestSnapshotManager`, `TestWapWorkflow`, `TestTransaction`, `TestFastAppend`,
`TestMergeAppend`, `TestOverwrite`, `TestReplacePartitions`, `TestRowDelta` and
`TestRESTCatalog`, with no failures.
Since this is the same precondition and the same lines, I'm happy for it to
go in here if you want to fold it in. Or, if you'd rather keep this PR focused,
I can open a follow-up PR stacked on this one. Whichever you and the reviewers
prefer.
_AI disclosure: the analysis, test and fix were generated with Claude Code
(Claude Opus 5.5). I've worked through the bug, the fix and the test against
the code, and run them locally, so this is partially reviewed: I follow it and
believe it's right, but I'm leaning on the AI for parts of the reasoning._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
