Copilot commented on code in PR #2343:
URL: https://github.com/apache/iceberg-go/pull/2343#discussion_r4405471699
##########
table/table.go:
##########
@@ -684,18 +684,37 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
remove := slices.Collect(maps.Keys(rebuiltLists))
if committed {
remove = orphanedManifests
- for _, u := range updates {
- if su, ok := u.(*addSnapshotUpdate); ok &&
su.supersededSource != nil {
- remove = append(remove,
su.supersededSource.supersededManifests()...)
- }
+ }
+ for _, u := range updates {
+ su, ok := u.(*addSnapshotUpdate)
+ if !ok || su.written == nil {
+ continue
}
+ if committed {
+ remove = append(remove,
su.written.uncommitted(su.Snapshot.ManifestList)...)
+ } else {
+ remove = append(remove,
su.written.discardRebuilt()...)
+ }
+ }
+ if committed && len(remove) > 0 {
+ referenced, err := committedFiles(wfs, updates)
+ if err != nil {
+ log.Printf("Warning: skipping cleanup of
uncommitted files: %v", err)
+
+ return
+ }
+ remove = slices.DeleteFunc(remove, func(path string)
bool {
+ _, ok := referenced[path]
+
+ return ok
+ })
}
// The commit may have failed because ctx was canceled; remove
the
// orphans without that cancellation so they are not left
behind.
cleanupCtx := context.WithoutCancel(ctx)
Review Comment:
Before iterating `remove` and calling `Remove`, it would be safer to
de-duplicate the list. Even rare duplicates can cause unnecessary IO and may
generate misleading warnings (e.g., a successful delete followed by a second
delete that returns not-found). A tangible fix is to normalize `remove` into a
set (map) or sort + compact it before the deletion loop.
##########
table/table.go:
##########
@@ -1034,6 +1053,28 @@ func stagedManifestLists(updates []Update)
map[string]struct{} {
return lists
}
+// committedFiles returns the manifest lists and manifests of every snapshot
+// updates add.
Review Comment:
The `committedFiles` doc comment is grammatically unclear (“snapshots
updates add”). Consider rewording to something unambiguous like “...of every
snapshot added by the provided updates” to make it easier to understand what
callers can rely on.
##########
table/snapshot_producers.go:
##########
@@ -64,12 +64,64 @@ type producerImpl interface {
needsValidation() bool
}
-// supersededAccumulator marks a producer that writes merged manifests OCC
-// retries orphan. Only rewrite implements it.
-type supersededAccumulator interface {
- // supersededManifests returns paths of merged manifests that earlier
- // passes wrote and a later pass replaced, to delete once the commit
lands.
- supersededManifests() []string
+// writtenManifests records the manifests a producer wrote for its staged
+// snapshot and each rebuild. Staged manifests are deleted only after a commit
+// lands, and a failed commit deletes only what its rebuilds wrote.
+type writtenManifests struct {
+ stagedList string
+ staged map[string]struct{}
+ unreferenced []string
+ rebuilt map[string]struct{}
+ keepRebuilt bool
+}
Review Comment:
The field name `unreferenced` is ambiguous: it’s not ‘unreferenced
globally’, but rather ‘written by the producer and not included in the staged
manifest list’. Renaming it to something more specific (e.g., `stagedUnlisted`
/ `stagedNotInList`) would reduce the chance of misinterpreting the cleanup
semantics later.
--
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]
