You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2024/07/05 22:52:24 UTC
[PR] Avoids looping when evaluating single set of authorizations [accumulo-access]
keith-turner opened a new pull request, #71:
URL: https://github.com/apache/accumulo-access/pull/71
Simplified the code to avoid looping when there is a single set of authorizations. This change was made in an attempt to improve performance and it did very slightly.
Before this change saw the following.
```
Benchmark Mode Cnt Score Error Units
AccessExpressionBenchmark.measureEvaluation thrpt 12 13.609 ± 0.300 ops/us
```
And after this change saw the following.
```
Benchmark Mode Cnt Score Error Units
AccessExpressionBenchmark.measureEvaluation thrpt 12 13.806 ± 0.254 ops/us
```
The performance improvement is slight so maybe not worthwhile. However this change simplified the evaluator code a good bit, so that is worthwhile.
--
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] Avoids looping when evaluating single set of authorizations [accumulo-access]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #71:
URL: https://github.com/apache/accumulo-access/pull/71#discussion_r1669063409
##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -149,7 +153,32 @@ static AccessEvaluator of(Authorizer authorizer) {
*
*/
static AccessEvaluator of(Collection<Authorizations> authorizationSets) {
- return new AccessEvaluatorImpl(AccessEvaluatorImpl.convert(authorizationSets));
+ List<AccessEvaluatorImpl> evaluators = new ArrayList<>(authorizationSets.size());
+ for (Authorizations authorizations : authorizationSets) {
+ evaluators.add(new AccessEvaluatorImpl(authorizations));
+ }
+
+ return new AccessEvaluator() {
Review Comment:
Ok, I think I understand this PR better now. You have modified `AccessEvaluatorImpl` such that it only operates on a single `Authorizations` object and moved the function of dealing with multiples into this anonymous object. I have no issue with this, but wondering if we should/could move this anonymous class to a concrete class, call it `AccessEvaluatorsImpl`, `MultiAccessEvaluatorImpl`, or something like that.
--
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] Avoids looping when evaluating single set of authorizations [accumulo-access]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #71:
URL: https://github.com/apache/accumulo-access/pull/71#discussion_r1669279988
##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -149,7 +153,32 @@ static AccessEvaluator of(Authorizer authorizer) {
*
*/
static AccessEvaluator of(Collection<Authorizations> authorizationSets) {
- return new AccessEvaluatorImpl(AccessEvaluatorImpl.convert(authorizationSets));
+ List<AccessEvaluatorImpl> evaluators = new ArrayList<>(authorizationSets.size());
+ for (Authorizations authorizations : authorizationSets) {
+ evaluators.add(new AccessEvaluatorImpl(authorizations));
+ }
+
+ return new AccessEvaluator() {
Review Comment:
> but wondering if we should/could move this anonymous class to a concrete class, call it AccessEvaluatorsImpl, MultiAccessEvaluatorImpl, or something like that.
Yeah can do that, it would keep the interface file source code really compact and it makes stacktraces a bit more readable.
--
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] Avoids looping when evaluating single set of authorizations [accumulo-access]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #71:
URL: https://github.com/apache/accumulo-access/pull/71#issuecomment-2211488127
Was looking at #69 and noticed how that code mimicked the existing code with a loops like the following for the Accumulo code. Was thinking the inner loop is usually a singleton list and it would be nice to get rid of the loop when it is a singleton. That is what this PR does.
```java
for(var expression : expressions) {
for(var evaluator : evaluators){
// evaluate expression using evaluator
}
}
```
--
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] Avoids looping when evaluating single set of authorizations [accumulo-access]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #71:
URL: https://github.com/apache/accumulo-access/pull/71#discussion_r1668956730
##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -149,7 +153,32 @@ static AccessEvaluator of(Authorizer authorizer) {
*
*/
static AccessEvaluator of(Collection<Authorizations> authorizationSets) {
- return new AccessEvaluatorImpl(AccessEvaluatorImpl.convert(authorizationSets));
+ List<AccessEvaluatorImpl> evaluators = new ArrayList<>(authorizationSets.size());
+ for (Authorizations authorizations : authorizationSets) {
+ evaluators.add(new AccessEvaluatorImpl(authorizations));
+ }
+
+ return new AccessEvaluator() {
Review Comment:
> If this returns an implementation of AccessEvaluator, do we still need AccessEvaluatorImpl? If so, do we end up having two different implementations?
Yeah there are two impls of the interface now. Still having AccessEvaluatorImpl makes it really easy for the multiple auths impl to use the single auths impl, so thinking it simplifies the code.
--
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] Avoids looping when evaluating single set of authorizations [accumulo-access]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #71:
URL: https://github.com/apache/accumulo-access/pull/71
--
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] Avoids looping when evaluating single set of authorizations [accumulo-access]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #71:
URL: https://github.com/apache/accumulo-access/pull/71#discussion_r1667205676
##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -149,7 +153,32 @@ static AccessEvaluator of(Authorizer authorizer) {
*
*/
static AccessEvaluator of(Collection<Authorizations> authorizationSets) {
- return new AccessEvaluatorImpl(AccessEvaluatorImpl.convert(authorizationSets));
+ List<AccessEvaluatorImpl> evaluators = new ArrayList<>(authorizationSets.size());
+ for (Authorizations authorizations : authorizationSets) {
+ evaluators.add(new AccessEvaluatorImpl(authorizations));
+ }
+
+ return new AccessEvaluator() {
Review Comment:
If this returns an implementation of AccessEvaluator, do we still need AccessEvaluatorImpl? If so, do we end up having two different implementations?
--
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