Add configurable in-memory caching to the HTTP Credentials Provider #155
Conversation
| @@ -37,7 +37,6 @@ protected void setup(Binder binder) | |||
| HttpCredentialsProvider.class, | |||
| innerBinder -> { | |||
| configBinder(innerBinder).bindConfig(HttpCredentialsProviderConfig.class); | |||
| innerBinder.bind(HttpCredentialsProvider.class); | |||
| @@ -57,8 +66,16 @@ public static class Filter | |||
| public TestingTrinoAwsProxyServer.Builder filter(TestingTrinoAwsProxyServer.Builder builder) | |||
| { | |||
| TestingHttpServer httpCredentialsServer; | |||
| Map<String, String> expectedHeaders = ImmutableMap.<String, String>builder() | |||
There was a problem hiding this comment.
nit: could we use ImmutableMap.of(...)? Its less verbose
There was a problem hiding this comment.
I usually tend to prefer using the builder if there are more than 1 or 2 entries. The .of(...) syntax gets a bit messy IMHO above that and you end up relying on formatting to keep each key & its value on the same line, it otherwise is quite unreadable.
No strong opinion, happy to change it if you prefer!
| } | ||
|
|
||
| private static class HttpCredentialsServlet | ||
| extends HttpServlet | ||
| { | ||
| private final Map<String, String> expectedHeaders; | ||
| private AtomicInteger requestCounter; |
There was a problem hiding this comment.
| private AtomicInteger requestCounter; | |
| private final AtomicInteger requestCounter; |
vagaerg
force-pushed
the
add-http-creds-caching
branch
2 times, most recently
from
33bd633
to
c2356f7
Compare
| @@ -37,7 +37,6 @@ protected void setup(Binder binder) | |||
| HttpCredentialsProvider.class, | |||
| innerBinder -> { | |||
| configBinder(innerBinder).bindConfig(HttpCredentialsProviderConfig.class); | |||
| innerBinder.bind(HttpCredentialsProvider.class); | |||
There was a problem hiding this comment.
Consider a separate commit for this with an explanation
| @@ -48,13 +60,29 @@ public HttpCredentialsProvider(@ForHttpCredentialsProvider HttpClient httpClient | |||
| this.jsonCodec = requireNonNull(jsonCodec, "jsonCodec is null"); | |||
| this.httpCredentialsProviderEndpoint = config.getEndpoint(); | |||
| this.httpHeaders = ImmutableMap.copyOf(config.getHttpHeaders()); | |||
| this.credentialsCache = Caffeine.newBuilder() | |||
There was a problem hiding this comment.
If cacheTtl is zero, we wouldn't build or use the cache at all. i.e. it should be Optional.
There was a problem hiding this comment.
I was trying to use a zero-sized cache as a no-op, since it handles the cases where either the TTL or the max size are zero (either of which should result in no caching) nicely.
I agree it's perhaps a bit hacky, will change
credentialsProviderModule already binds the interface to a provider, we do not need to bind our implementation again. This was resulting in multiple instantiation.
vagaerg
force-pushed
the
add-http-creds-caching
branch
from
c2356f7
to
a5132f7
Compare
No description provided.