zeroshade commented on code in PR #2351:
URL: https://github.com/apache/iceberg-go/pull/2351#discussion_r4415055423
##########
table/transaction.go:
##########
@@ -909,7 +909,15 @@ func (t *Transaction) ExpireSnapshots(opts
...ExpireSnapshotsOpt) error {
// Only add the update if there are actually snapshots to delete
if len(snapsToDelete) > 0 {
- updates = append(updates,
NewRemoveSnapshotsUpdate(snapsToDelete, cfg.postCommit))
+ // Build can set the last-updated time, but only apply may
change the
+ // transaction's builder.
+ planned, err := meta.clone().Build()
Review Comment:
Non-blocking: each `ExpireSnapshots` call clones and builds the whole
metadata and keeps it until commit. On tables with long histories that's a full
copy per expiry. Cleanup only needs the expired snapshots and their statistics
and partition-statistics entries, so recording just those would be enough.
##########
table/transaction.go:
##########
@@ -2662,11 +2670,20 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
// otherwise a retry would re-run the catalog commit.
t.committed = true
+ var removals []*removeSnapshotsUpdate
for _, u := range meta.updates {
+ if removal, ok := u.(*removeSnapshotsUpdate); ok {
+ removals = append(removals, removal)
+
+ continue
+ }
if perr := u.PostCommit(ctx, t.tbl, tbl); perr != nil {
err = errors.Join(err, perr)
}
}
+ if perr := deleteExpiredFiles(ctx, t.tbl, tbl, removals...);
perr != nil {
Review Comment:
Non-blocking: please link the retry follow-up (replanning expiration on
retry) to #2339 so the known error after a successful commit doesn't get lost.
##########
table/updates.go:
##########
@@ -664,30 +675,16 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx
context.Context, preTable *Table,
filesToDelete := make(map[string]struct{})
- for _, snapId := range u.SnapshotIDs {
- snap := preTable.Metadata().SnapshotByID(snapId)
- if snap == nil {
- return errors.New("snapshot should never be nil")
+ var expired []*Snapshot
+ for _, u := range removals {
+ if !u.postCommit {
Review Comment:
Non-blocking: when expirations with and without cleanup are mixed, a file
shared between the two kinds is deleted, while a file referenced only by the
no-cleanup expiry is kept. That seems right, but a short comment here or a test
would pin it down.
--
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]
