[GitHub] [accumulo-access] ctubbsii commented on a diff in pull request #16: Updates documentation and removes TODOs

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #16:
URL: https://github.com/apache/accumulo-access/pull/16#discussion_r1340683904


##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -117,15 +131,31 @@ interface AuthorizationsBuilder {
      *
      *
      */
-    ExecutionBuilder authorizations(Collection<Authorizations> authorizations);
+    OptionalBuilder authorizations(Collection<Authorizations> authorizations);
 
-    ExecutionBuilder authorizations(String... authorizations);
+    /**
+     * Allows specifying a single set of authorizations.
+     */
+    OptionalBuilder authorizations(String... authorizations);
 
-    ExecutionBuilder authorizations(Authorizer authorizer);
+    /**
+     * Allows specifying an authorizer that is analogous to a single set of authorization.
+     */
+    OptionalBuilder authorizations(Authorizer authorizer);
   }
 
-  interface ExecutionBuilder extends FinalBuilder {
-    ExecutionBuilder cacheSize(int cacheSize);
+  interface OptionalBuilder extends FinalBuilder {
+
+    /**
+     * When set to a value greater than zero, the result of evaluating expressions will be
+     * remembered and if the same expression is seen it again the remembered result will be used
+     * instead of reevaluating it. If this method is not called on the builder then no caching is
+     * done. When the same expressions can occur repeatedly caching can greatly increase
+     * performance.
+     *
+     * @param cacheSize the number of expressions evaluations to remember in an LRU cache.
+     */
+    OptionalBuilder cacheSize(int cacheSize);

Review Comment:
   Instead of having a built-in internal cache with only an option for a fixed size, I wonder if it'd be better to just have a pluggable cache, so users can provide whatever cache implementation they want, rather than only control the size.



##########
src/main/java/org/apache/accumulo/access/AccessExpression.java:
##########
@@ -88,7 +88,9 @@ static AccessExpression of(String expression) throws IllegalAccessExpressionExce
    * @param expression is expected to be encoded using UTF-8
    */
   static AccessExpression of(byte[] expression) throws IllegalAccessExpressionException {
-    return new AccessExpressionImpl(expression);
+    byte[] copy = new byte[expression.length];
+    System.arraycopy(expression, 0, copy, 0, expression.length);
+    return new AccessExpressionImpl(copy);

Review Comment:
   ```suggestion
       return new AccessExpressionImpl(Arrays.copyOf(expression, expression.length));
   ```



-- 
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