adutra commented on code in PR #17914:
URL: https://github.com/apache/iceberg/pull/17914#discussion_r4553182249
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -279,7 +281,7 @@ public SdkHttpFullRequest sign(
.build();
Key cacheKey = Key.from(remoteSigningRequest);
- SignedComponent cachedSignedComponent =
SIGNED_COMPONENT_CACHE.getIfPresent(cacheKey);
+ SignedComponent cachedSignedComponent =
signedComponentCache().getIfPresent(cacheKey);
Review Comment:
Not per sign request, but per `FileIO` instance. And we could have hundreds
if not thousands of those in the same JVM.
A freshly-created Caffeine cache cost little memory, but if the cache is per
`FileIO`, this means that 2 `FileIO`s belonging to the same principal cannot
share the cache, which results in more calls to the sign endpoint than
necessary.
Instead, I would keep the cache static, but would change the cache key.
But adding `token` or `client:secret` only to the cache key is not enough.
Other auth managers may include other types of credentials, and since 1.12,
catalog servers can also send credential-like data in the
`remote-signing-config` object: these should be part of the cache key as well.
As an extra protection, I think that the endpoint URI should also be part of
the cache key.
In conclusion, the cache key should include the whole `properties()` map.
Two clients with identical properties are indistinguishable to any signing
server, and can safely keep sharing cache entries. The signing endpoint, the
credentials and the remote signing configuration are all derived from them, and
are thus included automatically.
Including the whole properties map doesn't hurt the cache efficiency either
(or just marginally), because these properties are immutable for a given
`FileIO` instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
