Skip to content

AWS: Check commit status after failed commit if AWS client performed retries#7198

Merged
jackye1995 merged 4 commits into
apache:masterfrom
ChristinaTech:aws-availability-fix
Apr 10, 2023
Merged

AWS: Check commit status after failed commit if AWS client performed retries#7198
jackye1995 merged 4 commits into
apache:masterfrom
ChristinaTech:aws-availability-fix

Conversation

@ChristinaTech
Copy link
Copy Markdown
Contributor

This PR fixes the issue described by #7151 where a commit can succeed but Glue or Dynamo backed tables see a failure and delete the now-committed metadata.

@github-actions github-actions Bot added the AWS label Mar 25, 2023
@ChristinaTech
Copy link
Copy Markdown
Contributor Author

Confirmed this doesn't break the existing integration tests. Going to add some additional ones to better replicate the scenario this is designed to fix.

Comment thread aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java Outdated
@ChristinaTech ChristinaTech marked this pull request as ready for review March 29, 2023 01:31
@ChristinaTech ChristinaTech requested a review from jackye1995 April 4, 2023 00:38
Copy link
Copy Markdown
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @ChristinaTech ! Left some comments/questions, thanks for the contribution!

Comment thread aws/src/main/java/org/apache/iceberg/aws/util/RetryDetector.java
Comment thread aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbTableOperations.java Outdated
Copy link
Copy Markdown
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks for the fix!

Copy link
Copy Markdown
Contributor

@JonasJ-ap JonasJ-ap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jackye1995
Copy link
Copy Markdown
Contributor

Thanks for the work @ChristinaTech ! Thanks for the reviews @amogh-jahagirdar @JonasJ-ap !

@jackye1995 jackye1995 merged commit 9f3707b into apache:master Apr 10, 2023
@ChristinaTech ChristinaTech deleted the aws-availability-fix branch April 10, 2023 21:07
@ryanyuan
Copy link
Copy Markdown

Thanks @ChristinaTech.

@jackye1995 @amogh-jahagirdar @JonasJ-ap Could you please include this fix in the next release?

ericlgoodman pushed a commit to ericlgoodman/iceberg that referenced this pull request Apr 12, 2023
manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023
Copy link
Copy Markdown

@mohitgargk mohitgargk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants