cmcarthur commented on PR #1455: URL: https://github.com/apache/iceberg-rust/pull/1455#issuecomment-2992463463
ok, thanks again @CTTY for the review. I think this is ready for another look. changes: - Added tests to cover more realistic scenarios where new snapshots reference new and old manifests, ensuring that old manifests are not deleted if they are still referenced. - Removed the visibility change to `apply` in `transaction/mod.rs`. - Refactored `ExpireSnapshotAction` to impl `TransactionAction`. Separated out (and renamed) the remaining bit to ExpireSnapshotProcedure I am quite happy with how the result is factored. There are three primary entrypoints: - `ExpireSnapshotsProcedureBuilder` can be used to build an expire snapshots procedure. It's also callable from Table e.g. `table.expire_snapshots()....build()`. On build, you get a configured `ExpireSnapshotsProcedure` - You can then call `ExpireSnapshotsProcedure.execute(table, catalog)` to run the procedure in full - There is a lower level `ExpireSnapshotsAction` that implements `TransactionAction` and can be used to simply remove snapshots from a table Next steps I'm considering: - Perhaps now that the procedure does not hold a table, it would make sense to remove the `table.expire_snapshots()` shorthand - Should `ExpireSnapshotsAction` actually be `RemoveSnapshotsAction`? @CTTY can you give this another look? -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
