fapifta commented on code in PR #4547:
URL: https://github.com/apache/ozone/pull/4547#discussion_r1171018172
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/DefaultSecretKeyVerifierClient.java:
##########
@@ -44,37 +45,45 @@ public class DefaultSecretKeyVerifierClient implements
SecretKeyVerifierClient {
private static final Logger LOG =
LoggerFactory.getLogger(DefaultSecretKeyVerifierClient.class);
- private final LoadingCache<UUID, ManagedSecretKey> cache;
+ private final LoadingCache<UUID, Optional<ManagedSecretKey>> cache;
DefaultSecretKeyVerifierClient(SCMSecurityProtocol scmSecurityProtocol,
ConfigurationSource conf) {
Duration expiryDuration = parseExpiryDuration(conf);
Duration rotateDuration = parseRotateDuration(conf);
- long cacheSize = expiryDuration.toMillis() / rotateDuration.toMillis() + 1;
- CacheLoader<UUID, ManagedSecretKey> loader =
- new CacheLoader<UUID, ManagedSecretKey>() {
+ // if rotation is 1d, and each keys is valid for 7d before expiring,
+ // the expected number valid keys at any time is 7.
+ long expectedValidKeys =
+ expiryDuration.toMillis() / rotateDuration.toMillis() + 1;
+ // However, we want to cache some expired keys as well, to avoid requesting
+ // SCM for recently expire secret keys.
+ long cacheSize = expectedValidKeys * 2;
Review Comment:
Can we name the constant here? Currently the comment describes why we use a
multiplication here, instead of that it is possible to let the code speak if we
introduce a constant, name it, and then use it as the cacheSize and
expiryDuration multiplier. It does not have to be an instance level constant, a
local variable is fine for now, and I don't think we ever want to make it
configurable... So this is just a suggestion for clarity.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/DefaultSecretKeyVerifierClient.java:
##########
@@ -44,37 +45,45 @@ public class DefaultSecretKeyVerifierClient implements
SecretKeyVerifierClient {
private static final Logger LOG =
LoggerFactory.getLogger(DefaultSecretKeyVerifierClient.class);
- private final LoadingCache<UUID, ManagedSecretKey> cache;
+ private final LoadingCache<UUID, Optional<ManagedSecretKey>> cache;
DefaultSecretKeyVerifierClient(SCMSecurityProtocol scmSecurityProtocol,
ConfigurationSource conf) {
Duration expiryDuration = parseExpiryDuration(conf);
Duration rotateDuration = parseRotateDuration(conf);
- long cacheSize = expiryDuration.toMillis() / rotateDuration.toMillis() + 1;
- CacheLoader<UUID, ManagedSecretKey> loader =
- new CacheLoader<UUID, ManagedSecretKey>() {
+ // if rotation is 1d, and each keys is valid for 7d before expiring,
+ // the expected number valid keys at any time is 7.
+ long expectedValidKeys =
+ expiryDuration.toMillis() / rotateDuration.toMillis() + 1;
Review Comment:
Instead of +1 may we use Math.ceil()?
--
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]
