[GitHub] [nifi] markap14 commented on issue #3518: NIFI-6322: Introduced EvaluationContext to store state while making evaluator tree reusable

[GitBox <gi...@apache.org>]

markap14 commented on issue #3518: NIFI-6322: Introduced EvaluationContext to store state while making evaluator tree reusable
URL: https://github.com/apache/nifi/pull/3518#issuecomment-501414244
 
 
   Hey @FrederikP thanks for submitting this! It always makes me very happy when someone digs into the deep end on the framework stuff :) This is definitely a different approach than in PR 3277. I think this has some big benefits over PR 3277, but I think it has a few 'cons' as well. 
   
   Basically, I would say that the approaches boil down to:
   PR 3277: Try to avoid recreating Evaluators. But if there is an Evaluator that is stateless, recreate them.
   This PR: Never recreate Evaluators. Work around the issue of state by externalizing it, so that the evaluator need not be recreated.
   
   The nice thing about this PR is that it actually offers even better performance gain than 3277, especially as you noted around stateful Evaluators.
   The main con, though, is that it separates the trivial state that is held by Evaluators from the Evaluators themselves, pushing it into an external HashMap. This leads to more garbage collection (definitely worth the trade-off) but also muddies the API a little bit, resulting in additional 'State' types of classes for the Evaluators, and requiring that the `EvaluationContext` be pushed down throughout the whole stack.
   
   So it got me thinking - can we accomplish the main goal of this PR - to allow Evaluators never to be recreated, by ensuring that they have fresh 'state' each time, without the need to push down the `EvaluationContext` and delegate to a map?
   So I tried something a little different, that I think takes the best parts of this PR and the best parts of PR 3277 and combines them into one, and posted that as PR 3531 (https://github.com/apache/nifi/pull/3531). Here, it just introduces a new `reset()` method on the Evaluator and calls that each time that the `Expression` is to be evaluated. It means that the Evaluator can keep its state as member variables, results in a very small PR that changes little code, is just as fast as this PR, and doesn't change the Evaluator API.
   
   Do you mind taking a look at that PR and see if you agree with it? I do believe it marries up both PR's well.
   Also of note, you added some really great unit tests in this PR. I copied & pasted them in to verify that all of the tests pass, but then I removed them because I want to make sure that the code is attributed to you. So if you do agree that PR 3531 is the way to go, I'd appreciate it if you added in those Unit Tests on top, so that they are included and attributed to you.
   
   Thanks again!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services