hcrosse opened a new pull request, #2343: URL: https://github.com/apache/iceberg-go/pull/2343
Follows up #2100. **CI:** the `table` tests fail to compile on main until #2340 merges. This PR doesn't touch those call sites. I ran the full suite, race tests and lint with #2340's change applied, and they pass. ## What Merge-append and overwrite rebuild their manifests against the fresh parent on every retry and every resumed `Commit`. `doCommit` cleaned up only the manifest lists those passes wrote. A retried commit that landed left attempt 0's manifests, and each superseded rebuild's, on storage with nothing referencing them. A failed commit also left the manifests its rebuilds wrote. Each snapshot producer now records the manifests it writes for its staged snapshot and for each rebuild. A manifest counts as the producer's when it carries the producer's snapshot ID. This replaces the rewrite producer's `superseded` list. `doCommit` uses the record as follows: - When a commit lands, it deletes the recorded manifests that no committed snapshot references, attempt 0's included. - After a known failure, it deletes the manifests this call's rebuilds wrote and keeps the staged ones. `RewriteManifests` keeps its rebuilt manifests as well, since a resume can reuse them. - After an unknown commit state, it deletes nothing, as before. Before deleting anything after a commit lands, `doCommit` reads every committed snapshot's manifest list and keeps the files they reference. ## Why Java cleans up the same way on success. `SnapshotProducer.commit` reads the committed snapshot's manifests, deletes the uncommitted ones, and skips cleanup if it can't read them ([SnapshotProducer.java](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L548-L576)). `MergingSnapshotProducer.cleanUncommittedAppends` deletes an added manifest that a merge consumed once the commit succeeds, with or without a retry ([MergingSnapshotProducer.java](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L1120-L1132)). The failure path differs on purpose. Java's `cleanAll` deletes the staged manifests after a failure ([SnapshotProducer.java](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L612-L618)). Here a failed `Commit` leaves the transaction resumable and the next `Commit` can reuse them, so they stay. ## Behaviour changes - If a committed manifest list can't be read, cleanup is skipped entirely and the commit still succeeds. Before, superseded manifest lists were deleted without reading anything. - A commit that lands now reads its committed snapshots' manifest lists whenever it has files to clean up. That happens after any retry, and on the first attempt when a merge consumed the added manifests. ## Testing `table/retry_manifest_cleanup_test.go` covers merge-append and overwrite with deletes: - `TestCommitLeavesOnlyCommittedFiles`: on the first attempt and after two conflicts, storage holds exactly the committed snapshots' files, and each one is readable. - `TestFailedCommitKeepsOnlyStagedFilesUntilResumeLands`: after a failed `Commit`, only the staged files remain. The resumed `Commit` then leaves only committed files. - `TestCommitCleanupSkippedWhenCommittedListUnreadable`: nothing is deleted when a committed manifest list can't be read. - `TestUnknownCommitStateDeletesNoManifests`: nothing is deleted when the commit state is unknown. - `TestChainedSnapshotsKeepEveryCommittedManifest`: a fast append followed by a merging append in one transaction keeps the manifest only the first snapshot references. Without the fix, `TestCommitLeavesOnlyCommittedFiles` fails in every case except overwrite on the first attempt, and the resume and chained tests fail too. With the check against committed files removed, 15 tests fail, including overwrite after two conflicts, where the rebuild reuses a staged manifest. -- 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]
