You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2023/09/07 15:46:58 UTC
[GitHub] [iceberg] amogh-jahagirdar opened a new pull request, #8520: Core: Clear deleted files set between commit attempts for simple transactions
amogh-jahagirdar opened a new pull request, #8520:
URL: https://github.com/apache/iceberg/pull/8520
For transactions, a `deletedFiles` set is maintained which keeps track of the files to delete as part of the most recent commit. For simple transactions, this set is not being cleared between attempts to commit the transaction. It could be possible that on the first transaction attempt, an operation marks certain files for deletion. When the transaction fails and is retried those files may actually be used, and when the retry succeeds those files are cleaned up when they should not be.
The set should preserve the invariant of representing the set of files to clear after a transaction is committed, thus this PR clears the set between transaction commit attempts.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#discussion_r1330824697
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -421,6 +421,7 @@ private void commitSimpleTransaction() {
.onlyRetryOn(CommitFailedException.class)
.run(
underlyingOps -> {
+ deletedFiles.clear();
Review Comment:
Yes you're right, I stepped through the code a bit more, really the applyUpdates is more of a `reapplyUpdates` (in case the table state changed). `applyUpdates` is a no-op on the first attempt because the individual operations commit has already completed. If the deletedFiles set is cleared before the first commit and that commit succeeds we would never end up cleaning those files based on the cleanup done here https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L450. Although I don't know why our tests wouldn't pick up on this, we have tests which validate we cleanup the files we would expect
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue closed pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue closed pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
URL: https://github.com/apache/iceberg/pull/8520
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#issuecomment-1728017521
@amogh-jahagirdar pointed out that we already check and do not delete files that were committed. In that case, we want the set of delete files to be any file that was ever deleted and we don't need to clear the set. Thanks for digging in and finding the right answer, Amogh!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#discussion_r1330824697
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -421,6 +421,7 @@ private void commitSimpleTransaction() {
.onlyRetryOn(CommitFailedException.class)
.run(
underlyingOps -> {
+ deletedFiles.clear();
Review Comment:
Yes you're right, I stepped through the code a bit more, really the applyUpdates is more of a `reapplyUpdates` (in case the table state changed). `applyUpdates` is a no-op on the first attempt. If the deletedFiles set is cleared before the first commit and that commit succeeds we would never end up cleaning those files based on the cleanup done here https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L450. Although I don't know why our tests wouldn't pick up on this, we have tests which validate we cleanup the files we would expect
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#issuecomment-1710405905
Still need to add test cases
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#issuecomment-1714024844
@rdblue As I was looking into writing tests for this, I think in practice we end up not hitting the scenario described (at least as far as I understand). The reason is currently after the transaction is committed successfully the clean up only cleans up uncommitted files. https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L455 . The same logic exists at the `SnapshotProducer` level as well. https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L931 . So if a manifest file were to get reused and successfully committed in the transaction, it would be part of the `committedFiles` and never be deleted.
All that said, I think it's still probably a good invariant to have to clear the files to delete on every transaction attempt in case some new logic gets added in the future.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #8520: Core: Clear deleted files set between commit attempts for simple transactions
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#discussion_r1330618046
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -421,6 +421,7 @@ private void commitSimpleTransaction() {
.onlyRetryOn(CommitFailedException.class)
.run(
underlyingOps -> {
+ deletedFiles.clear();
Review Comment:
It's not safe to do this here because this will clear before the first commit. `applyUpdates` will not re-commit everything on the first commit because those are already completed. Instead I think this needs to be called within `applyUpdates` just before all operations are re-committed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org