laskoviymishka commented on code in PR #2542:
URL: https://github.com/apache/iceberg-go/pull/2542#discussion_r4577967399
##########
table/transaction_delete.go:
##########
@@ -177,6 +181,28 @@ func (t *Transaction) performMergeOnReadDeletion(ctx
context.Context, snapshotPr
dvs.removeDataFile(updater, df)
}
+ // A whole-file drop takes the file's deletion vector with it. Reject
any
+ // delete a concurrent commit adds against those files, so the drop can
+ // replay on a CAS conflict without stranding a peer's DV. The validator
+ // treats any concurrent equality delete as a conflict, so it covers
only
+ // files that carry a DV: drops of other files keep replaying as before.
+ //
+ // Only a delete that drops whole files and nothing else may replay. A
+ // partial delete adds a DV to a file that stays live, and nothing
checks
+ // that file for a concurrent DV, so replaying it could leave two live
DVs
+ // on one file. Such deletes keep the no-replay behavior the DV removal
+ // gives them.
+ var droppedWithDV []iceberg.DataFile
+ for _, df := range filesToDelete {
+ if _, ok := dvs[df.FilePath()]; ok {
+ droppedWithDV = append(droppedWithDV, df)
+ }
+ }
+ if len(droppedWithDV) > 0 && len(withPartialDeletions) == 0 {
Review Comment:
Should this check cover every dropped v3 file, not only `droppedWithDV`?
Same point as @badalprasadsingh's thread on this block, now with a repro: drop
a file with a DV and one without while a peer adds the first DV to the no-DV
file. On e9115e1f the stale delete commits on attempt 2 and leaves 1 live DV on
the removed file; `main` fails it with `ErrCommitFailed` on attempt 1. A
DV-only conflict check over every dropped file, rather than `rewriteValidator`,
would fix this and also close the no-DV drop gap `main` already has.
##########
website/src/concurrent-writes.md:
##########
@@ -143,7 +143,10 @@ the transaction is unusable.
After a conflict, calling `Commit` again refreshes the table and replays staged
changes even with retries disabled. Each call gets the configured retry
budget. A commit that removes delete files and cannot be replayed resubmits
-only if its branch has not moved.
+only if its branch has not moved. Copy-on-write deletes and overwrites, and
Review Comment:
Could we add an Unreleased compatibility note to `releases.md` for this?
These commits used to fail with `ErrCommitFailed` on the first conflict. Now
they replay, and when a peer removed the same file they end in
`ErrTransactionUnusable` wrapping `ErrCommitDiverged`
([here](https://github.com/apache/iceberg-go/blob/e9115e1f840632e6abadeef303d342fc53429998/table/table.go#L873-L875)),
which `errors.Is(err, ErrCommitFailed)` doesn't match. The godocs that still
say these commits are "not replayed" need the same update: `table.go` 356, 389
and 422, `transaction.go` 2481 and 2552, `transaction_delete.go` 313.
--
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]
