You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/12/10 00:41:23 UTC
[GitHub] [iceberg] rdblue opened a new pull request, #6399: API: Add strict metadata cleanup to SnapshotProducer
rdblue opened a new pull request, #6399:
URL: https://github.com/apache/iceberg/pull/6399
In most catalogs, the `CommitStateUnknownException` is used to signal to `SnapshotProducer` that it is not safe to clean up metadata files because they may have been committed. This introduces another option, "strict cleanup", that will only clean up metadata files if the commit fails with an exception that implements a marker interface, `CleanableFailure`.
The new strict mode allows catalogs to default to not cleaning up metadata files, unless the failure is known to be safe -- that the commit state is known. This is useful for the REST catalog, which allows plugging in alternative HTTP libraries. Because exceptions thrown by the REST HTTP client may change, it isn't possible to mark which ones signal that the commit state is unknown.
--
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 #6399: API: Add strict metadata cleanup to SnapshotProducer
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #6399:
URL: https://github.com/apache/iceberg/pull/6399#issuecomment-1703961597
Closing this in favor of #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] nastra commented on a diff in pull request #6399: API: Add strict metadata cleanup to SnapshotProducer
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6399:
URL: https://github.com/apache/iceberg/pull/6399#discussion_r1049921925
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -396,7 +399,9 @@ public void commit() {
} catch (CommitStateUnknownException commitStateUnknownException) {
throw commitStateUnknownException;
} catch (RuntimeException e) {
- Exceptions.suppressAndThrow(e, this::cleanAll);
+ if (!strictCleanup || e instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(e, this::cleanAll);
Review Comment:
just curious, would we need to check for `strictCleanup` in https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L453-L463 as well?
--
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 #6399: API: Add strict metadata cleanup to SnapshotProducer
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6399:
URL: https://github.com/apache/iceberg/pull/6399#discussion_r1314026358
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -396,7 +399,9 @@ public void commit() {
} catch (CommitStateUnknownException commitStateUnknownException) {
throw commitStateUnknownException;
} catch (RuntimeException e) {
- Exceptions.suppressAndThrow(e, this::cleanAll);
+ if (!strictCleanup || e instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(e, this::cleanAll);
Review Comment:
Yes! Good catch. Amogh is fixing this in the updated 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-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] nastra commented on a diff in pull request #6399: API: Add strict metadata cleanup to SnapshotProducer
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6399:
URL: https://github.com/apache/iceberg/pull/6399#discussion_r1049921925
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -396,7 +399,9 @@ public void commit() {
} catch (CommitStateUnknownException commitStateUnknownException) {
throw commitStateUnknownException;
} catch (RuntimeException e) {
- Exceptions.suppressAndThrow(e, this::cleanAll);
+ if (!strictCleanup || e instanceof CleanableFailure) {
+ Exceptions.suppressAndThrow(e, this::cleanAll);
Review Comment:
just curious, would we need check for `strictCleanup` in https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L453-L463 as well?
--
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 #6399: API: Add strict metadata cleanup to SnapshotProducer
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue closed pull request #6399: API: Add strict metadata cleanup to SnapshotProducer
URL: https://github.com/apache/iceberg/pull/6399
--
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