AWS: handle s3 and glue exceptions more gracefully as user errors#5304
AWS: handle s3 and glue exceptions more gracefully as user errors#5304jackye1995 merged 8 commits intoapache:masterfrom
Conversation
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks! Other than some nit comments, do you mind updating the title and description so it's a bit more clear what we're trying to handle in terms of s3 and glue exception, and why we should handle these exceptions differently then the other Runtime exceptions.
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogCommitFailure.java
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogCommitFailure.java
Show resolved
Hide resolved
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
Outdated
Show resolved
Hide resolved
xingfanx
changed the title
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
Outdated
Show resolved
Hide resolved
| @@ -149,6 +150,15 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) { | |||
| fullTableName, persistFailure); | |||
| commitStatus = checkCommitStatus(newMetadataLocation, metadata); | |||
There was a problem hiding this comment.
checkCommitStatus is a heavy operation, I think the check of AwsServiceException can happen before that?
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
looks good to me, thanks for the fix!
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix!
| fullTableName, persistFailure); | ||
| commitStatus = checkCommitStatus(newMetadataLocation, metadata); | ||
|
|
||
| if (persistFailure instanceof AwsServiceException) { |
There was a problem hiding this comment.
Can this be a separate catch block to simplify the exception handling flow? like
} catch (AwsServiceException serviceException) {
...
} catch (RuntimeException runtimeException) {
...
| @@ -147,7 +148,17 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) { | |||
| } catch (RuntimeException persistFailure) { | |||
| LOG.error("Confirming if commit to {} indeed failed to persist, attempting to reconnect and check.", | |||
There was a problem hiding this comment.
I think this logging should be moved around since checkCommitStatus is now conditional
4 participants
handle s3 and glue exceptions more gracefully as user errors so it's not being retried indefinitely and causing internal server errors
Tested with unittests