laskoviymishka commented on code in PR #2140:
URL: https://github.com/apache/iceberg-go/pull/2140#discussion_r4232301604
##########
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:
The last of the three round-1 assertions,
`NotErrorIs(ErrConflictingDeleteFiles)`, is still missing. It's the only one
that pins that the conflict validators stayed silent and the abort came from
`checkRemovedFiles` rather than a validator. It's vacuous today, since the
concurrent delete adds no delete files so the validators return nil regardless,
so I'd add it as a regression guard against a future reorder, or drop a reply
on the thread saying why it's redundant. One line:
```suggestion
require.NotErrorIs(t, err,
table.ErrCommitFailed)
require.NotErrorIs(t, err,
table.ErrConflictingDeleteFiles)
```
##########
table/transaction.go:
##########
@@ -2496,12 +2496,12 @@ func (t *Transaction) performCopyOnWriteDeletion(ctx
context.Context, operation
// without this check a refresh-and-replay would swap the original file
// for a rewrite built from the stale snapshot, dropping the concurrent
// deletes and resurrecting their rows. Mirrors Java's copy-on-write
- // validateNoConflictingDeletes: no isolation gating.
- removed := append(slices.Clip(filesToDelete), filesToRewrite...)
+ // validateNoConflictingDeletes: no isolation gating. A concurrent
+ // removal of one of these files is caught earlier, by the retry
+ // rebuild's checkRemovedFiles (ErrCommitDiverged).
+ removed := slices.Concat(filesToDelete, filesToRewrite)
if len(removed) > 0 {
- t.addValidator(func(cc *conflictContext) error {
- return validateNoNewDeletesForRewrittenFiles(cc,
removed)
- })
+ t.addValidator(rewriteValidator(removed))
Review Comment:
while we're here, the CoW path now routes through `rewriteValidator`, which
lives in the compaction code and reads as rewrite-specific, while this comment
names only `checkRemovedFiles`. A future addition to the rewrite validator
would then apply to Delete/Overwrite silently. I'd name `rewriteValidator` in
the comment so the coupling is visible. Behavior's identical today, this is
just about intent.
--
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]
