fix: Retry sign blob call with exponential backoff #1452
Conversation
| @@ -279,9 +279,6 @@ public TokenVerifier build() { | |||
| /** Custom CacheLoader for mapping certificate urls to the contained public keys. */ | |||
| static class PublicKeyLoader extends CacheLoader<String, Map<String, PublicKey>> { | |||
| private static final int DEFAULT_NUMBER_OF_RETRIES = 2; | |||
There was a problem hiding this comment.
curious if there is specific guideline for TokenVerifier to default retry 2 times?
There was a problem hiding this comment.
I don't think so. I think these values were randomly selected and used. I've kept it the same for now and we can re-visit retries again in the future.
westarle
left a comment
There was a problem hiding this comment.
This looks pretty good, I would like to see verification in the test:
- success case makes a single request
- 401s are not retried
- 50x errors are retried according to policy to failure
- 50x errors are retried according to policy to success
Yep, those test cases make sense. I was running into some limitations of MockIAMCredentialsServiceTransport class where I couldn't specify that some 5xx errors should be retried and couldn't control how many times it would retry for. I made some changes that to the class to support retrying and controlling the number of retry attempts. Those changes ended up touch more than just the SignBlob RPC so I made that into Part 2. I believe this PR only covers cases 1 and 2, but 3 and 4 should be tested in the second PR. |
|
Fixes #1451
The overall changes will be split into two parts. This part is part 1 where retries will be enabled and tests to cover successful and failed calls.
Part 2 will add additional tests to cover retries for the IAM Mock Server. Split it out to a separate PR since the IAM Mock Server will need to be refactored to support retries and variable number of responses. The refactor will touch multiple files that are non-related to adding retries to the Sign Blob RPC.
Changes: