-
Notifications
You must be signed in to change notification settings - Fork 35
Use the AWS SDK backoff logic for backoff and jitter #170
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: v2
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #170 +/- ##
============================================
+ Coverage 90.86% 99.15% +8.28%
- Complexity 90 101 +11
============================================
Files 6 6
Lines 230 237 +7
Branches 24 21 -3
============================================
+ Hits 209 235 +26
+ Misses 18 0 -18
+ Partials 3 2 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
harsheejshah
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.
testing closing an object approving
src/main/java/com/amazonaws/secretsmanager/caching/cache/SecretCacheObject.java
Show resolved
Hide resolved
src/main/java/com/amazonaws/secretsmanager/caching/cache/SecretCacheObject.java
Show resolved
Hide resolved
| /** | ||
| * The default TTL for an item stored in cache before access causing a refresh. | ||
| */ | ||
| public static final Duration DEFAULT_CACHE_ITEM_TTL_DURATION = Duration.ofHours(1); |
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.
Do we need this public? imo we can define this as private.
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.
I made it match DEFAULT_CACHE_ITEM_TTL for backwards compatibility, that way if customers are using it they have a path forward.
| * This prevents continuous refreshNow() calls by adding a random sleep. | ||
| */ | ||
| public static final long DEFAULT_FORCE_REFRESH_JITTER = 100; | ||
| public static final Duration DEFAULT_FORCE_REFRESH_JITTER_DURATION = Duration.ofMillis(100); |
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.
Do we need this public? imo its used internally so we can make it private
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.
I made it match DEFAULT_FORCE_REFRESH_JITTER for backwards compatibility, that way if customers are using it they have a path forward.
Issue #, if available:
Description of changes:
Updated the library to use Duration and Instant to represent time amounts.
The current backoff and retry logic has a long overflow bug here. This only comes into play if AWS Secrets Manager is inaccessible for hours at a time.
Switched the custom backoff and retry logic to backoff and jitter helpers provided by the AWS SDK for Java v2 which tops out the backoff at the SDK default of 20s. Added unit tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.