This is an automated email from the ASF dual-hosted git repository.
domgarguilo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo-access.git
The following commit(s) were added to refs/heads/main by this push:
new 8036595 Remove caching functionality (#25)
8036595 is described below
commit 8036595571ccc2a3ee3854327d4d95ef00ab8161
Author: Dom G <domgargu...@apache.org>
AuthorDate: Fri Oct 13 14:04:16 2023 -0400
Remove caching functionality (#25)
* Remove caching functionality
* Update javadoc to recommend external caching
---------
Co-authored-by: Keith Turner <ktur...@apache.org>
---
.../apache/accumulo/access/AccessEvaluator.java | 32 +++++-------
.../accumulo/access/AccessEvaluatorImpl.java | 23 ++------
.../accumulo/access/CachingAccessEvaluator.java | 61 ----------------------
.../accumulo/access/AccessEvaluatorTest.java | 7 ---
4 files changed, 18 insertions(+), 105 deletions(-)
diff --git a/src/main/java/org/apache/accumulo/access/AccessEvaluator.java
b/src/main/java/org/apache/accumulo/access/AccessEvaluator.java
index 3343200..ecf1f3e 100644
--- a/src/main/java/org/apache/accumulo/access/AccessEvaluator.java
+++ b/src/main/java/org/apache/accumulo/access/AccessEvaluator.java
@@ -24,8 +24,16 @@ import java.util.Collection;
* <p>
* Used to decide if an entity with one more sets of authorizations can access
zero or more access
* expression.
- *
* <p>
+ * <p>
+ * Note: For performance improvements, especially in cases where expressions
are expected to repeat,
+ * it's recommended to wrap this evaluator with an external caching mechanism,
such as Guava's
+ * cache, to leverage its extensive caching options. Caching is only safe
under the assumption that
+ * for an AccessEvaluator instance, evaluating the same expression multiple
times will always yield
+ * the same result. When considering caching, any environmental factors that
might change this
+ * assumption may need to be mitigated.
+ * </p>
+ *
* Below is an example that should print false and then print true.
*
* <pre>
@@ -78,7 +86,7 @@ public interface AccessEvaluator {
interface AuthorizationsBuilder {
- OptionalBuilder authorizations(Authorizations authorizations);
+ FinalBuilder authorizations(Authorizations authorizations);
/**
* Allows providing multiple sets of authorizations. Each expression will
be evaluated
@@ -131,31 +139,17 @@ public interface AccessEvaluator {
*
*
*/
- OptionalBuilder authorizations(Collection<Authorizations> authorizations);
+ FinalBuilder authorizations(Collection<Authorizations> authorizations);
/**
* Allows specifying a single set of authorizations.
*/
- OptionalBuilder authorizations(String... authorizations);
+ FinalBuilder authorizations(String... authorizations);
/**
* Allows specifying an authorizer that is analogous to a single set of
authorization.
*/
- OptionalBuilder authorizations(Authorizer authorizer);
- }
-
- 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);
+ FinalBuilder authorizations(Authorizer authorizer);
}
interface FinalBuilder {
diff --git a/src/main/java/org/apache/accumulo/access/AccessEvaluatorImpl.java
b/src/main/java/org/apache/accumulo/access/AccessEvaluatorImpl.java
index b815a61..c27e9da 100644
--- a/src/main/java/org/apache/accumulo/access/AccessEvaluatorImpl.java
+++ b/src/main/java/org/apache/accumulo/access/AccessEvaluatorImpl.java
@@ -152,12 +152,11 @@ class AccessEvaluatorImpl implements AccessEvaluator {
return
authorizedPredicates.stream().allMatch(accessExpression.aeNode::canAccess);
}
- private static class BuilderImpl implements AuthorizationsBuilder,
FinalBuilder, OptionalBuilder {
+ private static class BuilderImpl implements AuthorizationsBuilder,
FinalBuilder {
private Authorizer authorizationsChecker;
private Collection<List<byte[]>> authorizationSets;
- private int cacheSize = 0;
private void setAuthorizations(List<byte[]> auths) {
setAuthorizations(Collections.singletonList(auths));
@@ -179,14 +178,14 @@ class AccessEvaluatorImpl implements AccessEvaluator {
}
@Override
- public OptionalBuilder authorizations(Authorizations authorizations) {
+ public FinalBuilder authorizations(Authorizations authorizations) {
setAuthorizations(authorizations.asSet().stream().map(auth ->
auth.getBytes(UTF_8))
.collect(toUnmodifiableList()));
return this;
}
@Override
- public OptionalBuilder authorizations(Collection<Authorizations>
authorizationSets) {
+ public FinalBuilder authorizations(Collection<Authorizations>
authorizationSets) {
setAuthorizations(authorizationSets
.stream().map(authorizations -> authorizations.asSet().stream()
.map(auth -> auth.getBytes(UTF_8)).collect(toUnmodifiableList()))
@@ -195,14 +194,14 @@ class AccessEvaluatorImpl implements AccessEvaluator {
}
@Override
- public OptionalBuilder authorizations(String... authorizations) {
+ public FinalBuilder authorizations(String... authorizations) {
setAuthorizations(Stream.of(authorizations).map(auth ->
auth.getBytes(UTF_8))
.collect(toUnmodifiableList()));
return this;
}
@Override
- public OptionalBuilder authorizations(Authorizer authorizationChecker) {
+ public FinalBuilder authorizations(Authorizer authorizationChecker) {
if (authorizationSets != null) {
throw new IllegalStateException("Cannot set checker and
authorizations");
}
@@ -210,15 +209,6 @@ class AccessEvaluatorImpl implements AccessEvaluator {
return this;
}
- @Override
- public OptionalBuilder cacheSize(int cacheSize) {
- if (cacheSize < 0) {
- throw new IllegalArgumentException();
- }
- this.cacheSize = cacheSize;
- return this;
- }
-
@Override
public AccessEvaluator build() {
if (authorizationSets != null ^ authorizationsChecker == null) {
@@ -232,9 +222,6 @@ class AccessEvaluatorImpl implements AccessEvaluator {
accessEvaluator = new AccessEvaluatorImpl(authorizationSets);
}
- if (cacheSize > 0) {
- accessEvaluator = new CachingAccessEvaluator(accessEvaluator,
cacheSize);
- }
return accessEvaluator;
}
diff --git
a/src/main/java/org/apache/accumulo/access/CachingAccessEvaluator.java
b/src/main/java/org/apache/accumulo/access/CachingAccessEvaluator.java
deleted file mode 100644
index 414b051..0000000
--- a/src/main/java/org/apache/accumulo/access/CachingAccessEvaluator.java
+++ /dev/null
@@ -1,61 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.access;
-
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import java.util.LinkedHashMap;
-import java.util.Map;
-
-class CachingAccessEvaluator implements AccessEvaluator {
-
- private final AccessEvaluator accessEvaluator;
- private final LinkedHashMap<String,Boolean> cache;
-
- CachingAccessEvaluator(AccessEvaluator accessEvaluator, int cacheSize) {
- if (cacheSize <= 0) {
- throw new IllegalArgumentException();
- }
- this.accessEvaluator = accessEvaluator;
- this.cache = new LinkedHashMap<>(cacheSize, 0.75f, true) {
- private static final long serialVersionUID = 1L;
-
- @Override
- public boolean removeEldestEntry(Map.Entry<String,Boolean> entry) {
- return size() > cacheSize;
- }
- };
- }
-
- @Override
- public boolean canAccess(String expression) throws IllegalArgumentException {
- return cache.computeIfAbsent(expression, accessEvaluator::canAccess);
- }
-
- @Override
- public boolean canAccess(byte[] expression) throws IllegalArgumentException {
- return canAccess(new String(expression, UTF_8));
- }
-
- @Override
- public boolean canAccess(AccessExpression expression) throws
IllegalArgumentException {
- return cache.computeIfAbsent(expression.getExpression(),
- k -> accessEvaluator.canAccess(expression));
- }
-}
diff --git a/src/test/java/org/apache/accumulo/access/AccessEvaluatorTest.java
b/src/test/java/org/apache/accumulo/access/AccessEvaluatorTest.java
index 6a9e19e..97d7119 100644
--- a/src/test/java/org/apache/accumulo/access/AccessEvaluatorTest.java
+++ b/src/test/java/org/apache/accumulo/access/AccessEvaluatorTest.java
@@ -89,13 +89,6 @@ public class AccessEvaluatorTest {
evaluator =
AccessEvaluator.builder().authorizations(testSet.auths[0]).build();
runTestCases(testSet, evaluator);
- evaluator =
AccessEvaluator.builder().authorizations(testSet.auths[0]).cacheSize(1).build();
- runTestCases(testSet, evaluator);
-
- evaluator =
-
AccessEvaluator.builder().authorizations(testSet.auths[0]).cacheSize(10).build();
- runTestCases(testSet, evaluator);
-
Set<String> auths =
Stream.of(testSet.auths[0]).collect(Collectors.toSet());
evaluator =
AccessEvaluator.builder().authorizations(auths::contains).build();
runTestCases(testSet, evaluator);
