hcrosse opened a new issue, #2339: URL: https://github.com/apache/iceberg-go/issues/2339
## Feature Request / Improvement I'd like `Transaction.ExpireSnapshots` to recompute expiration against refreshed metadata when a commit retries. Since #2301, expiration selects snapshots once, stages a `RemoveSnapshotsUpdate` with fixed IDs, and pins the refs it walks. With `commit.retry.num-retries` greater than zero, a changed pinned ref causes `ErrCommitFailed` after the first refresh, before another submission. This is safe, but it also rejects changes that leave the expiration result unchanged or could be handled by replanning. #2100 and #2301 describe this as deliberate. I propose: - Keep the expiration options and the `nowMs` captured when `ExpireSnapshots` was called. - Generalize `rebuildSnapshotUpdates` so an update can rebuild itself against refreshed metadata. Replan expiration in transaction order, including any preceding staged append, rather than adding an expiration-specific case to `doCommit`. - Drop the pins added by expiration, since replanning replaces their protection. Keep explicit caller pins. - Use the same path when resuming a failed `Commit`, as described in #2100. Java [refreshes and recomputes expiration](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L196-L215) on [each commit attempt](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L360-L372). `BaseTransaction.applyUpdates` [recommits pending updates after refresh](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L439-L454). Java [captures `now` once](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L95-L96). Rust also [reloads and recommits each action](https://github.com/apache/iceberg-rust/blob/8119f796c461283f559d7b7354963ee3e44dac04/crates/iceberg/src/transaction/mod.rs#L220-L243), but its expiration planner [calls `Utc::now()` on each attempt](https://github.com/apache/iceberg-rust/blob/8119f796c461283f559d7b7354963ee3e44dac04/crates/iceberg/src/transaction/expire_snapshots.rs#L101-L117). I'd keep Go's original time so retries do not move the cutoff. I'd add SQLite catalog tests that assert the exact surviving snapshot IDs when a peer: - Appends to `main`. - Rolls `main` back. - Creates a branch that protects snapshots selected for expiration. - Appends while our transaction combines an append with an expiration that removes snapshots. I'd also test that retries reuse the original cutoff time. `RollbackToSnapshot` is out of scope and would keep its pin. Java [only rechecks that the target is an ancestor](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/SetSnapshotOperation.java#L91-L105), so it can roll back over a peer's append. Failing is safer for that operation. I'm happy to implement it if the approach is agreed. -- 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]
