kinolaev commented on PR #18509: URL: https://github.com/apache/iceberg/pull/18509#issuecomment-6383143761
> We should not check DVs for newly added data files to conflict with concurrent commits (they can't add the same data files and DVs for them). And if no files left to validate, then we can just skip the history crawling Thanks @pvary, agreed, PR #18626 implements exactly that. As far as I know, the Flink sinks only produce DVs for data files added in the same commit, so, as long as that stays true, the core fix covers the snapshot expiration failure for all three committers. I'll close this PR once #18626 is merged. > Would it make sense to add this applied-then-reported-failed case to the core tests? Thanks for reporting this, @zhengxiang378928908-code. You're right: on v3 tables, #18626 also changes the applied-then-reported-failed retry when all DVs reference data files added by the same operation. The retry passes this check and, unless another validation fails, takes the no-op path for an already-committed snapshot. That helps `IcebergCommitter` and `IcebergFilesCommitter`, but not `DynamicCommitter`, where `MaxCommittedCheckpointIdValidator` still fails the retry. I think the test belongs in #18407, which proposes the general fix for such retries. -- 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]
