AWS: Enable Adaptive Retries for AWS KMS client by hsiang-c · Pull Request #11420 · apache/iceberg

hsiang-c · 2024-10-29T06:19:28Z

We're reaching the limit of AWS KMS requests per AWS account quota and we'd like to enable retry in KMS client to be more resilient to failures.
This PR is similar to AWS: Set better defaults for S3 retry behaviour #11052 but using the SDK provided RetryMode.
I didn't choose RetryPolicy b/c it is deprecated.

hsiang-c · 2024-10-29T08:36:40Z

@nastra @jackye1995 please review it when you have time, thanks!

jbonofre

It looks good to me. I just left a nit about pointing to the documentation (especially about retry mode accepted values).

jbonofre · 2024-10-29T08:52:55Z

aws/src/main/java/org/apache/iceberg/aws/kms/KmsClientProperties.java

+    this.kmsRetryMode = RetryMode.valueOf(KMS_RETRY_MODE_DEFAULT);
+  }
+
+  public KmsClientProperties(Map<String, String> properties) {


nit: I would add the link to AWS configBuilder configuration, especially about retry mode (to know the accepted values for retry mode).

@jbonofre Thank you for your review! I linked to both RetryMode and ClientOverrideConfiguration.

danielcweeks · 2024-11-05T23:20:06Z

Is this really necessary to make configurable? Why shouldn't we just set ADAPTIVE_V2 as the default when constructing the client? This is only for retry behavior, I'm not sure it's worth even making it an option.

hsiang-c · 2024-11-07T03:59:21Z

@danielcweeks Thank you for your review.

Is this really necessary to make configurable?

I am also working on an enhancement to Iceberg S3 doc: #11321, which makes me think users might want to configure AWS service clients with available configurations provided by the client libraries.

This is only for retry behavior, I'm not sure it's worth even making it an option.

Got it, I have no problem with setting ADAPTIVE_V2 as the default, though the default mode is LEGACY according to AWS Java V2 SDK.

Is having too many options a concern to you? I'd like to learn more about your thoughts.

Thanks again for your time.

danielcweeks · 2024-11-07T17:15:46Z

Is having too many options a concern to you? I'd like to learn more about your thoughts.

I feel like the proliferation of options is not a good thing unless there's real value being added. This doesn't seem to be one of those cases. Rather than adding a lot of boilerplate configuration, I would suggest just setting this at ADAPTIVE_V2, which seems to be an improvement upon the legacy default.

hsiang-c · 2024-11-08T04:07:30Z

Thanks for your feedback @danielcweeks Please take a look again.

If you still have time, I have another PR for you #11321

amogh-jahagirdar

Thanks @hsiang-c I agree with @danielcweeks that we need to be cautious when adding new properties, specifically what that means is evaluating if it's a valuable knob that's worth adding. In this case we know that we generally want to retry when throttled and the recommended strategy is adaptive so the produced clients should just do that without exposing things to users.

Overall this PR looks to be a good direction, just had a comment that I don't think the public KmsClientProperties class is needed at the moment.

amogh-jahagirdar · 2024-11-13T01:56:55Z

aws/src/main/java/org/apache/iceberg/aws/kms/KmsClientProperties.java

+import software.amazon.awssdk.core.retry.RetryMode;
+import software.amazon.awssdk.services.kms.KmsClientBuilder;
+
+public class KmsClientProperties implements Serializable {


Do we need this class considering there's no separate properties we're introducing for KMS and the fact that it may make sense to also apply ADAPTIVE_V2 for Glue client as well (not needed in this PR, but down the line we could just use that)? Just don't want to avoid public properties classes until they're really necessary

I think an applyRetryConfigurations in the existing AwsClientProperties also would work well since that class is meant for generalizing across the different AWS clients.

@amogh-jahagirdar Thank you for your feedback.

Making applyRetryConfigurations as part of AwsClientProperties makes sense to me.

amogh-jahagirdar · 2024-12-10T04:34:28Z

aws/src/test/java/org/apache/iceberg/aws/kms/TestKmsClientProperties.java

+    awsClientProperties.applyRetryConfigurations(builder);
+    RetryMode retryPolicy = builder.overrideConfiguration().retryMode().get();
+
+    assertThat(retryPolicy).as("retry mode was not set").isEqualTo(RetryMode.ADAPTIVE_V2);


Nit: Could we clean up this error message to be "Retry mode should be ADAPTIVE_V2"?

Sorry I missed this one.

accidentally approved, there's some things I think we should cleanup before

amogh-jahagirdar

Thanks @hsiang-c I think this is very close just some java docs that I think should be cleaned up before merging

amogh-jahagirdar · 2024-12-10T04:40:54Z

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java

+  /**
+   * Set ADAPTIVE_V2 <a
+   * href="https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/core/retry/RetryMode.html">RetryMode</a>
+   * for an AWS client builder.
+   *
+   * <p>For more details, see
+   * https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/core/client/config/ClientOverrideConfiguration.Builder.html#retryStrategy(software.amazon.awssdk.core.retry.RetryMode)
+   */


Could we structure this JavaDoc similar to the the other apply configuration methods? e.g. checkout the java doc in applyClientCredentialConfigurations or applyClientRegionConfiguration

amogh-jahagirdar

Thanks @hsiang-c , this looks great to me now!

amogh-jahagirdar · 2024-12-11T01:19:55Z

I'll go ahead and merge, thanks @hsiang-c and thanks @jbonofre for reviewing

hsiang-c · 2024-12-11T02:18:37Z

Thank you @amogh-jahagirdar

Enable RetryMode for AWS KMS client

28b3b31

github-actions bot added the AWS label Oct 29, 2024

jbonofre approved these changes Oct 29, 2024

View reviewed changes

Comment on RetryMode

13321b7

Set retry mode to ADAPTIVE_V2

943f6b0

amogh-jahagirdar reviewed Nov 13, 2024

View reviewed changes

Move retry config to the generic AwsClientProperties class

cfa192d

amogh-jahagirdar previously approved these changes Dec 10, 2024

View reviewed changes

amogh-jahagirdar self-requested a review December 10, 2024 04:38

amogh-jahagirdar reviewed Dec 10, 2024

View reviewed changes

Address Amogh's review

fd2f8e8

amogh-jahagirdar approved these changes Dec 11, 2024

View reviewed changes

amogh-jahagirdar merged commit ff81344 into apache:main Dec 11, 2024
49 checks passed

amogh-jahagirdar changed the title ~~AWS: Enable RetryMode for AWS KMS client~~ AWS: Enable Adaptive Retries for AWS KMS client Dec 11, 2024

hsiang-c deleted the aws_kms_retry branch December 11, 2024 02:18

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

AWS: Enable RetryMode for AWS KMS client (apache#11420)

bd6049d

Conversation

hsiang-c commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsiang-c commented Oct 29, 2024

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

jbonofre Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

hsiang-c Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

danielcweeks commented Nov 5, 2024

Uh oh!

hsiang-c commented Nov 7, 2024

Uh oh!

danielcweeks commented Nov 7, 2024

Uh oh!

hsiang-c commented Nov 8, 2024

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

hsiang-c Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

hsiang-c Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hsiang-c commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hsiang-c commented Oct 29, 2024 •

edited

Loading

hsiang-c Dec 2, 2024 •

edited

Loading

amogh-jahagirdar commented Dec 11, 2024 •

edited

Loading