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 2024/05/24 15:04:39 UTC
[PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
amogh-jahagirdar opened a new pull request, #10373:
URL: https://github.com/apache/iceberg/pull/10373
Upstream callers of the transaction/snapshot producer APIs such as engines like Spark currently handle CommitStateUnknown exceptions to avoid cleaning up data files when it's unclear if the commit was successful or not. For example, see the rewrite data files procedure here: https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/actions/RewriteDataFilesCommitManager.java#L116
Currently, we only cleanup metadata files if strict cleanup is enabled (default true) and it's a cleanable failure. We should extend this to data file cleanups that upstream callers may do. To do that we can throw a commit state unknown exception in SnapshotProducer/BaseTransaction if strict cleanup is enabled and it's not a cleanable failure. Then engines won't go and cleanup data files.
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#discussion_r1645017182
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -333,6 +333,8 @@ private void commitCreateTransaction() {
// the commit failed and no files were committed. clean up each update
if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
cleanAllUpdates();
+ } else if (ops.requireStrictCleanup()) {
Review Comment:
No longer applicable. Just updated the approach; it's still conservative but it puts the handling more on the Spark integration side to explicitly handle CleanableFailure rather than the original approach which attempted to normalize the unknown cases to CommitStateUnknownException; I think it's better because that "normalization" may be quite hard and there maybe other cases.
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -333,6 +333,8 @@ private void commitCreateTransaction() {
// the commit failed and no files were committed. clean up each update
if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
cleanAllUpdates();
+ } else if (ops.requireStrictCleanup()) {
Review Comment:
No longer applicable. Just updated the approach; it's still conservative but it puts the handling more on the Spark integration side to explicitly handle CleanableFailure rather than the original approach which attempted to normalize the unknown cases to CommitStateUnknownException; I think it's better because that "normalization" may be quite hard to achieve in practice and there maybe other 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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2131598402
> This might cover a few cases where a RuntimeException is thrown, but I think the underlying problem is more that a lot of the implementations throw CommitFailedException or RuntimeExceptions for a wide variety of cases that may or may not be cleanable.
It probably helps to break down the exceptions in terms of intentions and whether or not the are cleanable. Then in the Java implementation we can establish a standard practice for catalog implementations.
--
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
Re: [PR] Core, Spark: Spark writes/actions should only perform cleanup if failure is cleanable [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2237525207
> instead of normalizing non-cleanable failures to CommitStateUnknownExcpetion now engines, in this case Spark will explicitly handle CleanableFailure
@amogh-jahagirdar thanks for making this change. sorry, I have been OOO for a while.
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2129842921
A few tests are failing now due to always throwing CommitStateUnknownException; I'll step through and see which exceptions should be marked as cleanable and which should be propagated as is. Glancing at the failures though it does seem like a good chunk should just be marked as cleanable
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "zackurey (via GitHub)" <gi...@apache.org>.
zackurey commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2134141718
👍 to the conservative approach being taken here to _not_ assume a failure is `Cleanable` unless explicitly categorized as such. I worry about the burden it puts on catalog implementations to categorize every error into either `CommitStateUnknown` or `CommitStateFailed`. We’ve encountered issues where edge cases manifest as generic RuntimeExceptions, which leads to inadvertent data file deletion. We run orphan file deletion frequently, so any data files that should have been reaped, but aren’t due to the change in behavior being introduced here, would have negligible COGs impact.
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "sumedhsakdeo (via GitHub)" <gi...@apache.org>.
sumedhsakdeo commented on code in PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#discussion_r1628766388
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -333,6 +333,8 @@ private void commitCreateTransaction() {
// the commit failed and no files were committed. clean up each update
if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
cleanAllUpdates();
+ } else if (ops.requireStrictCleanup()) {
Review Comment:
I believe this comment needs rewording:
https://github.com/apache/iceberg/blob/afc30818b734879e53a24f90f034685cf8fc56bc/core/src/main/java/org/apache/iceberg/TableOperations.java#L120
"would it now say if requiresStrictCleanup is true, we would NOT cleanup if it was not a cleanableFailure", is there a easier way to represent this if-else condition in that case."
+1 to being conservative we have been bitten multiple times with premature cleanup by spark because the code raised unchecked exceptions such as OutOfMemoryError. On that note, how are unchecked exceptions that derive from Error class handled from cleanup perspective.
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#discussion_r1645018496
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteManifestsSparkAction.java:
##########
@@ -355,8 +356,11 @@ private void replaceManifests(
// don't clean up added manifest files, because they may have been successfully committed.
throw commitStateUnknownException;
} catch (Exception e) {
- // delete all new manifests because the rewrite failed
- deleteFiles(Iterables.transform(addedManifests, ManifestFile::path));
+ if (e instanceof CleanableFailure) {
Review Comment:
Will need to backport this to 3.4/3.3
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#discussion_r1613637717
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -436,6 +440,8 @@ private void commitSimpleTransaction() {
} catch (RuntimeException e) {
if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
cleanUpOnCommitFailure();
+ } else if (ops.requireStrictCleanup()) {
+ throw new CommitStateUnknownException(e);
Review Comment:
Yeah I probably should, it's failing the cyclomatic check here
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2176908633
@stevenzwu I updated so that instead of normalizing non-cleanable failures to `CommitStateUnknownExcpetion` now engines, in this case Spark will explicitly handle `CleanableFailure`. It's a bit more implementation than the original approach since a few touchpoints in Spark integration needed to be updated but I think it makes more sense because in the original approach maybe there can be cases which still fail to actually get normalized to `CommitStateUnknown` exception (I think this is what @danielcweeks was getting at). Now it's explicit on the engine side we only cleanup if it's a cleanable failure.
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2173768003
Sorry for the delayed reply everyone, just getting back into this PR.
@stevenzwu
>I like the introduction of the CleanableFailure marker interface for this purpose. Engines can potentially updated to reflect that intention. On the other hand, the challenge is that this is not defined in the spec. It may be less obvious to engine/catalog implementer. Some engines (like Spark) have learned to handle CommitStateUnknownException. Can we standardize on CleanableFailure for Java implementation? Can we add the behavior clarification to the spec?
I think it's a bit hard to add to the spec because the spec should be generalizable to all programming langauges/implementations, where a Java exception does not really fit that mold. However, I do like the idea of standardizing the exception usage in the Java implementation and the JVM engine integrations!
>Seems that this PR is trying to standardize uncleanable commit errors to CommiteStateUnknownException.
I like the safer approach of explicitly defining cleanable errors (instead of defining uncleanable errors). Engines/catalogs can handle cleanable errors explicitly.
It is true that this PR is attempting to standardizing uncleanable commit errors to CommitStateUnknownException, but to be clear everything except CleanableFailure is considered CommitStateUnknownException which is intentional for guaranteeing safe cleanup.In the current implementation the standardization of uncleanable to `CommitStateUnknownException` was intentional since engines already handle that, and I was trying to avoid changing the different engine integrations. However, you bring up a good point that it may be better to go ahead and change the engines to explicitly handle CleanableFailure. I'll 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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#discussion_r1613626590
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -436,6 +440,8 @@ private void commitSimpleTransaction() {
} catch (RuntimeException e) {
if (!ops.requireStrictCleanup() || e instanceof CleanableFailure) {
cleanUpOnCommitFailure();
+ } else if (ops.requireStrictCleanup()) {
+ throw new CommitStateUnknownException(e);
Review Comment:
Maybe we can have a helper function cleanupIfPossible(e, () -> someCleanupFunction()) to reduce the duplication here
--
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
Re: [PR] Core, Spark: Spark writes/actions should only perform cleanup if failure is cleanable [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2237511439
@amogh-jahagirdar sorry, I have been OOO for a while
> it may be better to go ahead and change the engines to explicitly handle CleanableFailure. I'll look into this!
it will be great to have this as a follow 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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2135677546
> Upstream callers of the transaction/snapshot producer APIs such as engines like Spark currently handle CommitStateUnknown exceptions to avoid cleaning up data files when it's unclear if the commit was successful or not.
I like the introduction of the `CleanableFailure` marker interface for this purpose. Engines can potentially updated to reflect that intention. On the other hand, the challenge is that this is not defined in the spec. It may be less obvious to engine/catalog implementer. Some engines (like Spark) have learned to handle `CommitStateUnknownException`. Can we standardize on `CleanableFailure` for Java implementation? Can we add the behavior clarification to the spec?
Seems that this PR is trying to standardize **uncleanable** commit errors to `CommiteStateUnknownException`.
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#discussion_r1613629622
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -415,6 +415,8 @@ public void commit() {
} catch (RuntimeException e) {
if (!strictCleanup || e instanceof CleanableFailure) {
Exceptions.suppressAndThrow(e, this::cleanAll);
+ } else if (strictCleanup) {
+ throw new CommitStateUnknownException(e);
Review Comment:
Let me see about adding some tests, should be similar in principal to what I did in https://github.com/apache/iceberg/pull/8397/files#diff-37f64b622d5c3e0e890d94773e84ef49ceb3c1e9b0c03da313db9266b0e75afbR2051
--
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
Re: [PR] Core: Throw CommitStateUnknownException if RuntimeException that is not marked as cleanable is thrown [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #10373:
URL: https://github.com/apache/iceberg/pull/10373#issuecomment-2130322881
@amogh-jahagirdar this might cover a few cases where a RuntimeException is thrown, but I think the underlying problem is more that a lot of the implementations throw CommitFailedException or RuntimeExceptions for a wide variety of cases that may or may not be cleanable.
For example, I think JdbcTableOperations throws UncheckedSQLException for a lot of cases that are cleanable.
Overall, I'm in favor of deferring to not cleaning up, but I think due to the current exception handling, there are lots of edge 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