URI cache for DynamoDB account id based endpoint #6087
Conversation
…' into feature/master/endpoint-id-cache
…' into feature/master/endpoint-id-cache
|
| @@ -168,6 +168,10 @@ public int size() { | |||
| return cache.size(); | |||
| } | |||
|
|
|||
| public boolean contains(K key) { | |||
| private final LruCache<UriConstructorArgs, URI> cache = LruCache.builder(UriConstructorArgs::newInstance) | ||
| .maxSize(100) | ||
| .build(); | ||
|
|
| private static final SdkUri INSTANCE = new SdkUri(); | ||
|
|
||
| private final LruCache<UriConstructorArgs, URI> cache = LruCache.builder(UriConstructorArgs::newInstance) | ||
| .maxSize(100) |
There was a problem hiding this comment.
I thought 150 is what we decided. Can we create a constant and add a comment explaining why this magic number was chosen
| } | ||
|
|
||
| private void logCacheUsage(boolean containsKey, URI uri) { | ||
| if (log.isLoggingLevelEnabled(Level.TRACE)) { |
There was a problem hiding this comment.
log.isLoggingLevelEnabled is not required for SdkLogger
| StringConstructorArgs key = new StringConstructorArgs(s); | ||
| boolean containsK = cache.contains(key); | ||
| URI uri = cache.get(key); | ||
| logCacheUsage(containsK, uri); |
There was a problem hiding this comment.
Can we do cache.contains check in logCacheUsage?
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
Can we add tests for equals and hashcode for all of them?
|
|
||
| private static final SdkUri INSTANCE = new SdkUri(); | ||
|
|
||
| private final LruCache<UriConstructorArgs, URI> cache = LruCache.builder(UriConstructorArgs::newInstance) |
There was a problem hiding this comment.
Can we initialize it in SdkUri ctor? On a similar note, worth doing lazy initialization for SdkUri singleton? Note that we can use Lazy
| @@ -274,7 +278,11 @@ private void codegenTreeBody(RuleSetExpression expr) { | |||
| @Override | |||
| public Void visitEndpointExpression(EndpointExpression e) { | |||
| builder.add("return $T.endpoint(", typeMirror.rulesResult().type()); | |||
| builder.add("$T.builder().url($T.create(", Endpoint.class, URI.class); | |||
| if (endpointCaching) { | |||
| builder.add("$T.builder().url($T.getInstance().create(", Endpoint.class, SdkUri.class); | |||
There was a problem hiding this comment.
Can we add a codegen tests?
Actually, should we just do it for all service clients at this point? I don't really see us removing the logic ever...
| "type": "feature", | ||
| "category": "AWS SDK for Java v2", | ||
| "contributor": "", | ||
| "description": "URI cache for DynamoDB account id based endpoint" |
There was a problem hiding this comment.
Can we describe the behavior rather than implementation?
Motivation and Context
Add caching for account id based endpoint in front of the URI constructors
Modifications
SdkURIclass which caches calls to the various URI constructors methods.SdkURIclass in generated endpoint resolution code. Enabled this config only for DynamoDB.Testing
Types of changes