jasonf20 commented on PR #9230: URL: https://github.com/apache/iceberg/pull/9230#issuecomment-1859879328
Hi @rdblue. Thanks for info. Good to know about the manifest compaction conflict case. I was looking for a way the list could be partially cleared and this answers that. I have pushed another commit returning to my original fix with the `hasDelete` optimization. Some more notes/questions: 1. The transaction test I added did fail before the fix since the cleanup is called with `EMPTY_SET` anyway if the commit fails and the manifest list files are also cleaned up in `SnapshotProducer::cleanAll()`. The manifest lists being cleaned up required adding `forceReApply` fix and the manifest files are fixed by the change in `MergingSnapshotProducer`. I think this fix is still relevant as the test fails without it. Removing either of the fixes causes the test to fail. 2. I understand that calling `commit()` is only allowed by transaction in theory, but the API doesn't prevent this currently and I think it's safer to fix it for now. The only change that is specifically for this use case is the `forceReApply` I added to the transaction, the other changes are needed regardless. If we decide to block this at some point in the future the tests I added can be modified to verify that they get an invalid usage exception. What do you think? -- 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
