You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2023/10/13 16:22:10 UTC
[PR] Remove caching functionality [accumulo-access]
DomGarguilo opened a new pull request, #25:
URL: https://github.com/apache/accumulo-access/pull/25
Closes #17
This PR removes caching functionality and updates the javadoc to reflect its removal.
--
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
Re: [PR] Remove caching functionality [accumulo-access]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #25:
URL: https://github.com/apache/accumulo-access/pull/25#discussion_r1358598682
##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -24,8 +24,13 @@
* <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.
Review Comment:
Could add a caveat to the caching suggestion.
```suggestion
* cache, to leverage its extensive caching options. Caching is only safe under the assumption that for an AccessEvaluator instance that evaluating the same expression multiple times will always yield the same result. When considering caching, may need to mitigate any environmental factors that could change this assumption.
```
--
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
Re: [PR] Remove caching functionality [accumulo-access]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #25:
URL: https://github.com/apache/accumulo-access/pull/25
--
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
Re: [PR] Remove caching functionality [accumulo-access]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #25:
URL: https://github.com/apache/accumulo-access/pull/25#discussion_r1358488355
##########
src/test/java/org/apache/accumulo/access/AccessEvaluatorTest.java:
##########
@@ -89,11 +89,10 @@ public void runTestCases() throws IOException {
evaluator = AccessEvaluator.builder().authorizations(testSet.auths[0]).build();
runTestCases(testSet, evaluator);
- evaluator = AccessEvaluator.builder().authorizations(testSet.auths[0]).cacheSize(1).build();
+ evaluator = AccessEvaluator.builder().authorizations(testSet.auths[0]).build();
Review Comment:
These two tests runs can be removed because they are now repeating the same tests.
--
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
Re: [PR] Remove caching functionality [accumulo-access]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #25:
URL: https://github.com/apache/accumulo-access/pull/25#discussion_r1358491468
##########
src/test/java/org/apache/accumulo/access/AccessEvaluatorTest.java:
##########
@@ -89,11 +89,10 @@ public void runTestCases() throws IOException {
evaluator = AccessEvaluator.builder().authorizations(testSet.auths[0]).build();
runTestCases(testSet, evaluator);
- evaluator = AccessEvaluator.builder().authorizations(testSet.auths[0]).cacheSize(1).build();
+ evaluator = AccessEvaluator.builder().authorizations(testSet.auths[0]).build();
Review Comment:
Nice catch. Fixed in ea431d5
--
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