-
Notifications
You must be signed in to change notification settings - Fork 3k
AWS: set retry policy on glue client #15094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
singhpk234
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, i think dynamo is used both for cases like lockmanager as well as as a catalog so should be fine if we wanna use it
@edgao worst case you can use reflection ? i wonder if we can just test in the way we tested prev ones
|
This never was tested, though it probably should be. I'll defer to you on reflection - personally I don't love it (e.g. AWS changes the field name, now our test is broken). But I'll implement it tomorrow and we can see how it looks. Will add the retry policy on the dynamo client as well 👍 |
|
@singhpk234 how does this look? Looking through AwsClientFactories, I think we could probably also refactor it to accept a mocked |
8c5c0a6 to
1f27bd0
Compare
|
actually derp, I think I just found the right sequence of method calls to do this without reflection 🤦 one minute |
singhpk234
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @edgao !
similar to ff81344#diff-6c7387be14971c2febd327b91d20ef0e12fc705fc3e7a8449387d8496d418190, except on the
glue()method. We're seeing some rate-limit errors when operating against Glue, and the default retry policy (LEGACY, i.e. 3 retries + no backoff) probably isn't helping.I didn't add a new unit test - that PR adds a test to verify that
applyRetryConfigurationsworks correctly, but that's orthogonal to whether or not the method is actually called. TestAwsClientFactories returns the client object directly, and I couldn't figure out how to easily extract the retry policy from a GlueClient object.I could be convinced to add this invocation on the
dynamo()method as well - LMK