[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2707: Cached last 64 successful ZKSecurityTool.checkCryptPass password checks

[GitBox <gi...@apache.org>]

ctubbsii commented on code in PR #2707:
URL: https://github.com/apache/accumulo/pull/2707#discussion_r882241112


##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -157,6 +157,13 @@ public enum Property {
       "The permission handler class that accumulo will use to determine if a "
           + "user has privilege to perform an action",
       "1.5.0"),
+  INSTANCE_SECURITY_ZK_AUTH_CACHE_ENABLED("instance.security.authenticator.zk.cache.enabled",
+      "true", PropertyType.BOOLEAN,
+      "Enables the temporary caching of successfully authenticated"
+          + " user passwords in org.apache.accumulo.server.security.handler.ZKAuthenticator to"
+          + " mitigate the performance penalties of having to compute the password hash"
+          + " on every API call",
+      "2.1.0"),

Review Comment:
   Since this is an internal optimization, I don't want to bloat users with extra configuration. If in future, we evaluate this again, and it's no longer needed due to improvements in the JDK or in commons-codec, then this property will become OBE, and we'll have churn removing it.
   
   Also, it only applies to ZKAuthenticator, which is itself configurable. If they don't want this, they can just replace ZKAuthenticator with a different custom authenticator. So, there's already a control knob for this that is available to users. This option is effectively redundant, and it's confusing if they've used a custom authenticator already.
   



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org