Revanth14 commented on code in PR #2140:
URL: https://github.com/apache/iceberg-go/pull/2140#discussion_r4235478431
##########
table/cow_delete_conflict_test.go:
##########
@@ -198,3 +201,40 @@ func
TestCopyOnWriteConflict_ConcurrentDeleteOnUntouchedFileCommits(t *testing.T
}
}
}
+
+// TestCopyOnWriteConflict_ConcurrentRemovalOfSameFileDiverges pins the other
+// half of copy-on-write conflict detection: when a concurrent commit already
+// removed a data file this commit also removes, the stale commit aborts
+// terminally with ErrCommitDiverged (the retry rebuild's checkRemovedFiles)
+// rather than rebuilding the file from the stale snapshot, and the table keeps
+// the concurrent result.
+func TestCopyOnWriteConflict_ConcurrentRemovalOfSameFileDiverges(t *testing.T)
{
+ for _, version := range cowFormatVersions {
+ for _, isolation := range cowIsolations {
+ for _, op := range cowOps {
+ for _, removal := range cowRemovals {
+ name := fmt.Sprintf("v%s/%s/%s/%s",
version, isolation, op, removal.name)
+ t.Run(name, func(t *testing.T) {
+ ctx := context.Background()
+ tbl := appendTenRows(t,
newCoWConflictTestTable(t, version, isolation))
+
+ txn := stageCopyOnWrite(t, tbl,
op, removal.filter)
+
+ // A concurrent copy-on-write
delete of id==4 rewrites the
+ // same data file first.
+ _, err := stageCopyOnWrite(t,
tbl, "delete", iceberg.EqualTo(iceberg.Reference("id"), int64(4))).Commit(ctx)
+ require.NoError(t, err)
+
+ _, err = txn.Commit(ctx)
+ require.ErrorIs(t, err,
table.ErrCommitDiverged)
+ require.ErrorContains(t, err,
"no longer on the branch head")
+ require.NotErrorIs(t, err,
table.ErrCommitFailed)
Review Comment:
Left this out on purpose: `ErrConflictingDeleteFiles` wraps
`ErrCommitFailed`, so the `NotErrorIs(t, err, table.ErrCommitFailed)` line
above already fails in every case this one would. That includes a future
reorder where a validator fires first, and it also covers
`ErrConflictingDataFiles`, which is what fires here under serializable
isolation with `checkRemovedFiles` disabled.
It also can't fail in this scenario even after a reorder: the concurrent
commit adds no delete files, so the delete validator returns nil regardless.
Happy to add it for readability if you'd still prefer it spelled out.
--
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]
