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/08/26 03:51:11 UTC
[GitHub] [iceberg] amogh-jahagirdar opened a new pull request, #8397: Add strict metadata cleanup to TableOperation.
amogh-jahagirdar opened a new pull request, #8397:
URL: https://github.com/apache/iceberg/pull/8397
Co-authored-by: Ryan Blue <bl...@apache.org>
This is a continuation of https://github.com/apache/iceberg/pull/6399
Strict metadata cleanup will only trigger cleanups during commits when the commit fails with an exception which implements the marker interface CleanableFailure This will get used in SnapshotProducer and BaseTransaction and is useful for catalogs like the REST catalog where arbitrary HTTP client exceptions can be thrown (instead of the usual CommitStateUnknown exceptions).
--
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 #8397: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1306312176
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -474,6 +461,16 @@ private void commitSimpleTransaction() {
private void cleanUpOnCommitFailure() {
// the commit failed and no files were committed. clean up each update.
+ computeUncommittedFiles();
+
+ // delete all the uncommitted files
+ Tasks.foreach(deletedFiles)
+ .suppressFailureWhenFinished()
+ .onFailure((file, exc) -> LOG.warn("Failed to delete uncommitted file: {}", file, exc))
+ .run(ops.io()::deleteFile);
+ }
+
+ private void computeUncommittedFiles() {
Review Comment:
The diff looks a bit strange, but it's a bit of refactoring which separates
1) computing the files to clean up via going through all the updates and calling `cleanAll` which enqueues the uncommitted files
2.) Actually deleting the files
I did this because in the code there a references to "cleaning up" but really what's happening is an enqueuing of possible files to cleanup after the commit succeeds or fails.
--
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 #8397: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1306312176
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -474,6 +461,16 @@ private void commitSimpleTransaction() {
private void cleanUpOnCommitFailure() {
// the commit failed and no files were committed. clean up each update.
+ computeUncommittedFiles();
+
+ // delete all the uncommitted files
+ Tasks.foreach(deletedFiles)
+ .suppressFailureWhenFinished()
+ .onFailure((file, exc) -> LOG.warn("Failed to delete uncommitted file: {}", file, exc))
+ .run(ops.io()::deleteFile);
+ }
+
+ private void computeUncommittedFiles() {
Review Comment:
The diff looks a bit strange, but it's a bit of refactoring which separates
1) computing the files to clean up via going through all the updates and calling `cleanAll` which enqueues the uncommitted files
2.) Actually deleting the files
I did this because in the code there a rereferences to "cleaning up" but really what's happening is an enqueuing of possible files to cleanup after the commit succeeds or fails.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1317826909
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -316,20 +317,13 @@ private void commitCreateTransaction() {
} catch (CommitStateUnknownException e) {
throw e;
-
Review Comment:
Nit: unnecessary whitespace change. I also like the whitespace here because it maintains separation.
--
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 #8397: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#issuecomment-1694145834
I still need to write tests for this, putting it up for early eyes on the code
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1314015172
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -319,15 +320,7 @@ private void commitCreateTransaction() {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
-
+ computeUncommittedFiles();
Review Comment:
I think this needs to be refactored more. If the `RuntimeException` was something like a 500 error for a create table or replace table transaction, then the table may have been created or replaced. For strict cleanup, I think we still need to check for a cleanable exception in those cases.
I think these should be:
```java
} catch (RuntimeException e) {
if (!ops.requiresStrictCleanup() || e instanceof CleanableFailure) {
cleanUpOnCommitFailure();
}
}
Tasks.foreach(deletedFiles).run(ops.io()::delete);
```
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1318885267
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -380,14 +375,9 @@ private void commitReplaceTransaction(boolean orCreate) {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
+ if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
+ cleanUpOnCommitFailure();
Review Comment:
Ah the delete is best effort so the second time when the file no longer exists the delete would fail but we catch it and emit a warn log.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1314010173
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -436,6 +439,14 @@ public void commit() {
}
}
+ private void cleanupAfterFailure(RuntimeException exception) {
+ if (!strictCleanup || exception instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(exception, this::cleanAll);
+ }
+
+ throw exception;
Review Comment:
This shouldn't throw an exception. The exception should be thrown from the calling code so that it is clear when reading that section how execution will happen. It's fine to pass it in here to determine whether to clean up though.
I'm not sure that this refactoring is worth the method, but it's up to you once this `throw` is moved back.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1316210793
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -319,15 +320,7 @@ private void commitCreateTransaction() {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
-
+ computeUncommittedFiles();
Review Comment:
I've updated. @rdblue If I'm not missing anything we would still want to preserve the behavior of cleaning up the files in the `finally` block because that set of deleted files would the files from previous commit attempts (this was already happening regardless of commit outcome, including CommitUnknownException). We should be able to clean those files up in any situation.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1314030808
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -436,6 +439,14 @@ public void commit() {
}
}
+ private void cleanupAfterFailure(RuntimeException exception) {
+ if (!strictCleanup || exception instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(exception, this::cleanAll);
+ }
+
+ throw exception;
Review Comment:
Good point, it's not a good idea to have this helper method throw a runtime exception, it's less explicit to a reader since they'd have to actually step into the helper which is being called in the catch block. I'll fix this up
--
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 #8397: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1306568955
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1006,17 +1014,6 @@ public void testTableAuth(
.build();
};
- Mockito.doAnswer(addTableConfig)
Review Comment:
shouldn't have been removed
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#issuecomment-1707058898
Thanks for the review @rdblue, I've updated the PR for updating the create/replace cases to perform the strict metadata cleanup check. One case I wanted to double check was the deletion of files happening in the finally block for create/replace https://github.com/apache/iceberg/pull/8397#discussion_r1316210793 . If I understand right, we still would want to preserve this cleanup regardless of commit outcome or strict cleanup because that set of files would be files from previous commit attempts which we can safely cleanup.
> @amogh-jahagirdar, in addition to fixing this, I think we also need to look into file cleanup for transactions. I was reviewing the current handling and I noticed that the deletedFiles set is never cleared between retries for a SIMPLE transaction. As a result, there is a chance that a file that was deleted by one operation's commit but reused when that operation is recommitted will remain in the delete set and be removed.
I think all we need to do is to clear the delete file set before each commit attempt. Could you look into this in a follow-up PR?
Good catch, sure I will look into this!
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1316210793
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -319,15 +320,7 @@ private void commitCreateTransaction() {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
-
+ computeUncommittedFiles();
Review Comment:
I've updated. If I'm not missing anything we would still want to preserve the behavior of cleaning up the files in the `finally` block because that set of deleted files would the files from previous commit attempts (this was already happening regardless of commit outcome, including CommitUnknownException). We should be able to clean those files up in any situation.
--
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 #8397: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1306312314
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -436,6 +439,12 @@ public void commit() {
}
}
+ private void cleanupIfPossible(RuntimeException exception) {
+ if (!strictCleanup || exception instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(exception, this::cleanAll);
+ }
+ }
Review Comment:
I did a separate helper to avoid checkstyle complaining about cyclomatic complexity.
--
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 #8397: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1306491225
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -399,7 +402,7 @@ public void commit() {
} catch (CommitStateUnknownException commitStateUnknownException) {
throw commitStateUnknownException;
} catch (RuntimeException e) {
- Exceptions.suppressAndThrow(e, this::cleanAll);
+ cleanupIfPossible(e);
}
Review Comment:
This doesn't seem right, we still want the suppress and throw behavior but the cleanup should be conditional on the strictCleanup and CleanableFailure
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1316225139
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -353,6 +356,7 @@ protected TableMetadata refresh() {
}
@Override
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
Review Comment:
It didn't seem worth it to have the exception helper function just to avoid cyclomatic complexity. There's probably better refactoring we could do if we wanted to remove this, for now just decided to suppress warnings.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1318870175
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -380,14 +375,9 @@ private void commitReplaceTransaction(boolean orCreate) {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
+ if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
+ cleanUpOnCommitFailure();
Review Comment:
Otherwise, I think this looks good.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#issuecomment-1710574693
Thanks for review @rdblue ! merging
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1314031511
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -319,15 +320,7 @@ private void commitCreateTransaction() {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
-
+ computeUncommittedFiles();
Review Comment:
Right, it's possible that during the create/replace we get a 500 but the create/replace actually was successful. So we still need the cleanable failure check. I realized I'm also missing tests for create/replace/simple transactions so I'll add those
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1318059274
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -316,20 +317,13 @@ private void commitCreateTransaction() {
} catch (CommitStateUnknownException e) {
throw e;
-
Review Comment:
Sure, I added back the newline
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1318884428
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -380,14 +375,9 @@ private void commitReplaceTransaction(boolean orCreate) {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
+ if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
+ cleanUpOnCommitFailure();
Review Comment:
Oh yes good catch, we're duplicating the call to delete the files. Although I wonder how the create/replace tests for this case were passing.
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1314030808
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -436,6 +439,14 @@ public void commit() {
}
}
+ private void cleanupAfterFailure(RuntimeException exception) {
+ if (!strictCleanup || exception instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(exception, this::cleanAll);
+ }
+
+ throw exception;
Review Comment:
Good point, it's not a good idea to have a helper method throw a runtime exception, it's less explicit to a reader since they'd have to actually step into the helper. I'll fix this up
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#issuecomment-1710483728
Looks good to me. Let's merge when tests are passing. Thanks, @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-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 merged pull request #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #8397:
URL: https://github.com/apache/iceberg/pull/8397
--
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 #8397: API, Core: Add strict metadata cleanup to TableOperations
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8397:
URL: https://github.com/apache/iceberg/pull/8397#discussion_r1318868914
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -380,14 +375,9 @@ private void commitReplaceTransaction(boolean orCreate) {
} catch (RuntimeException e) {
// the commit failed and no files were committed. clean up each update.
- Tasks.foreach(updates)
- .suppressFailureWhenFinished()
- .run(
- update -> {
- if (update instanceof SnapshotProducer) {
- ((SnapshotProducer) update).cleanAll();
- }
- });
+ if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
+ cleanUpOnCommitFailure();
Review Comment:
Shouldn't this be `cleanAllUpdates`? Both `cleanUpOnCommitFailure` and the `finally` block are going to run the delete function on every file.
--
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