URI bounded cache for DynamoDB account id based endpoint #6228
Conversation
…' into feature/master/endpoint-id-cache
…' into feature/master/endpoint-id-cache
- added codegen tests - added equals & hash-code test for ConstructorArgs classes - fix cache size - SdkUri constructor - checkstyle complained about newURI -> renamed newUri
…' into feature/master/endpoint-id-cache
| @@ -153,15 +145,6 @@ private boolean isAccountIdUri(String s) { | |||
| return false; | |||
| } | |||
|
|
|||
| private void logCacheUsage(boolean containsKey, URI uri) { | |||
There was a problem hiding this comment.
Was logging only useful for LRU? Can we not benefit from seeing the usage of the BoundedCache as well?
There was a problem hiding this comment.
Removed previously since BoundedCache didn't have containsKey method, added method and added back logging
| * Otherwise, the value is calculated based on the supplied function {@link Builder#builder(Function)}. | ||
| */ | ||
| public V get(K key) { | ||
| V value = cache.get(key); |
There was a problem hiding this comment.
can key ever be null? should we add a null check avoid cache.get(null)?
There was a problem hiding this comment.
Currently, key will never be null when invoked internally by the SDK. Adding null check for future safeguard
| /** | ||
| * Clean up the cache by removing an unspecified entry | ||
| */ | ||
| private void cleanup() { |
There was a problem hiding this comment.
This cleanup implementation removes a single entry every time the cache is at capacity. That means it becomes less efficient when the cache becomes full because every new entry will follow with a cleanup() call.
Consider this:
- cache.put(key) (capacity 1)
- cache.put(key) (capacity 2)
- cache.put(key) (capacity 3)
...
...
...
- cache.put(key) (capacity 99)
- cache.put(key) (capacity 100)
- cache is at max capacity
- cleanup() removes 1
<cycle>
- cache.put(key) (capacity 99)
- cache.put(key) (capacity 100)
- cache is at max capacity
- cleanup() removes 1
- <cycle>
- cache.put(key) (capacity 99)
- cache.put(key) (capacity 100)
- cache is at max capacity
- cleanup() removes 1
Did we consider an alternative cleanup strategy?
There was a problem hiding this comment.
Good callout, updated to batch evict 10 entries when full
Motivation and Context
Add caching for account id based endpoint in front of the URI constructors
Continuation of #6087
LruCachewith a random-evictionBoundedCacheinSdkUriModifications
Testing
Screenshots (if appropriate)
Types of changes