ctubbsii commented on issue #56: URL: https://github.com/apache/accumulo-access/issues/56#issuecomment-1947751246
@keith-turner [wrote](#issuecomment-1945117107): > This is the [code](https://github.com/apache/accumulo-access/blob/009246e93aaa91dc4ce0b2a35ae8f14ddf04e210/src/main/java/org/apache/accumulo/access/AccessEvaluatorImpl.java#L198-L211) the does the evaluation. It seems like it could be decomposed, would lose the reuse of the thread local. That code used to be a stream instead of a for loop, but we discovered that creating a stream for evaluating each access expression was cause a huge performance hit when benchmarking. In case my use of Streams had any bearing on your comment, my example only used Streams for a concise expression of `allMatch` and `anyMatch`. Of course, the caller could use any functionally equivalent construction of their choosing, but it doesn't really have any bearing on my proposal. But, the performance cost of losing the reuse of internal reusable objects would certainly have a bearing on my proposal. @keith-turner [wrote](#issuecomment-1945145050): > Opened #61 after looking at code for this issue. For any changes made for this issue we can benchmark the changes before and after to see what the impact is. I haven't had a chance to play around with benchmarking my suggestions here, but if benchmarking shows that the difference is meaningful because of the reuse of internal objects that we'd be losing, then we could still have a cleaner single entry point through Authorizations, but we would have to do it differently to allow evaluation of multiple Authorizations from a single evaluator without using Evaluator itself as an entry point. For example: ```java // using an intermediate obj something like AuthorizationsSet Authorizations.of(auths1).and(Authorizations.of(auths2).evaluator(); Authorizations.of(auths1).or(Authorizations.of(auths2).evaluator(); // final obj is a new evaluator, that takes elements of the first, but updated to account for the additional authorizations to evalute Authorizations.of(auths1).evalutor().and(Authorizations.of(auths2)); Authorizations.of(auths1).evalutor().or(Authorizations.of(auths2)); // explicit use of an authorizations set AuthorizationsSet.of(auths1, auths2).evaluateForAll(); AuthorizationsSet.of(auths1, auths2).evaluateForAny(); ``` Each of these constructions could be implemented to keep the reuse optimizations, but without needing Evaluator as a separate API entry point. @dlmarion [wrote](#issuecomment-1944311859): > I don't think it would be this simple, there is some escape-handling that is occurring in the AccessEvaluatorImpl constructors. I'm not sure I understand this comment or how it relates to what I'm proposing. I think that the internal escape-handling implementation would be unaffected by anything I've suggested here. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
