hantangwangd commented on PR #10983: URL: https://github.com/apache/iceberg/pull/10983#issuecomment-2316827563
> The incremental cleanup logic is already quite complex and I'm now thinking it's not really worth it to add handling this particular case. There's probably more cases and then if we were to keep adding handling for that at some point the implementation will look like the reachable cleanup anyways. Yes, it is indeed the case, there's probably more cases. Considering the factors you mentioned, I agree with you that maybe it's better to accept that incremental cleanup has limitations and from an API perspective ensure that callers don't fall into a state where they cleanup files that are still referenced (as @rdblue suggested). > would you be open to changing the approach to just ensure that if expireSnapshotId is called and incrementalCleanup is set to true we fail before triggering the file cleanup? Also if incremental cleanup isn't specified I think incrementalCleanup should be set to false in the case a specific snapshotID is specified in expireSnapshotId. That achieves the safety guarantees I think we want. Sure, I'd like to make this change, and modify the test cases accordingly. Thanks for all your explanations and suggestions @amogh-jahagirdar . -- 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
