jasonf20 commented on PR #9230: URL: https://github.com/apache/iceberg/pull/9230#issuecomment-1861357164
> To which test case are you referring? I didn't think any of them were valid uses of the API because commit was called multiple times and the behavior is undefined. To the test cases, I added to `TestTransaction` that calls commit multiple times. I understand the behaviour is undefined but it works with this fix and without it, it changes the table state to a state that isn't queryable. I think the other option is to throw an exception but that might be a breaking change that you need to declare in the release. > What is the purpose of forceReApply? When `commit` is called again on a transaction that failed and was cleaned up it commits a manifest list that was deleted. This remembers that the cleanup was called and applies the changes of the transaction operations again to create a valid state that it can commit again. As mentioned above if we don't want to allow this we can not fix it but it creates a non-queryable state at the moment. > I think that we should focus on fixing the bug and then discuss cases where commit or commitTransaction are called multiple times. The important thing is getting a fix we can put in a patch release. Alright, I'll remove the `forceReApply` from the transaction and remove the test cases that call `commit` multiple times so this fix can get merged asap. The other code path does break the table though, but we can fix it in a separate PR. -- 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
