-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust ExpressionAggregationScript to support inter-segment concurrency #99667
Adjust ExpressionAggregationScript to support inter-segment concurrency #99667
Conversation
Handling of _value in a script agg does not support search concurrency when using the expression script engine. The reason is that the value gets set globally assuming sequential execution. This commit addresses that by setting the value to the values source associated with the correct leaf reader context, while it was previosly being set on a shared data structure. Closes elastic#99156
Pinging @elastic/es-search (Team:Search) |
if (fv.advanceExact(docId)) { | ||
return Explanation.match((float) fv.doubleValue(), "ReplaceableConstDoubleValues"); | ||
} | ||
return Explanation.noMatch("ReplaceableConstDoubleValues"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do aggregation scripts get explained? I am thinking this may never get called. If anyone knows how this gets called, that would help me figure out how to test this, otherwise I am thinking of throwing UnsupportedOperationException in this method, just to surface that it is never called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is also used for score scripts, so it does need to implement explain
I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you show me where? I can't find that usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I am perplexed, because the score usage does not pass the replaceable around, which makes it not replaceable as setValue will never be called. Effectively, will the value not always be 0
? Maybe @jdconrad can help us here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried replacing the replaceable usage in scoring with a constant and I got no failures: does it mean that it's not needed or that we have no test coverage for it? Also, if it is a constant (0) do we even need to add it to the bindings. All in all, I am thinking that we could clean things up and get rid of the explain here, as I don't find a way to test my changes to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should never be called for score scripts, since it is specifically for _value, which is not a thing there. However, I vaguely recall some issue when migrating to score script context, which caused me to add _value to score scripts.
Could you try throwing UnsupportedOperationException and see if anything fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this will never be called by anything. From what I can tell explain will only work on a script score query using Painless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReplaceableConstDoubleValueSource() { | ||
fv = new ReplaceableConstDoubleValues(); | ||
} | ||
private final Map<LeafReaderContext, ReplaceableConstDoubleValues> specialValues = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely happy with this solution, but given that we need a global data structure exposed in the bindings, which will retrieve values per segment, I think we don't have many other options than tracking this special value per segment. This needs to be a concurrent hash map because getValues and setValue are called concurrently from different threads (one per slice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also uneasy, and I think the longer-term solution here is that we need to build the aggregations per-slice rather than globally. But that's a much bigger change, so I think I'm happy with this as a quick fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this will significantly hurt performance on expression score scripts which also use this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will, because it would only try to access the map if using the value source, which will only occur if _value
appears in the score script, which is (I think) bogus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! What about aggregation scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_value is specifically part of aggregation scripts. But I do not believe explain would ever be called on an it in that context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need have a closer look at how aggregations interact with multithreading, but this is fine as an immediate fix.
ReplaceableConstDoubleValueSource() { | ||
fv = new ReplaceableConstDoubleValues(); | ||
} | ||
private final Map<LeafReaderContext, ReplaceableConstDoubleValues> specialValues = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also uneasy, and I think the longer-term solution here is that we need to build the aggregations per-slice rather than globally. But that's a much bigger change, so I think I'm happy with this as a quick fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too!
We have found some inconsistencies as part of #99667, around the current usages of ReplaceableConstDoubleValueSource in ExpressionsScriptEngine. It looks like _value is exposed to the bindings of score scripts, but setValue is never called hence it will always be 0. That can be replaced with a constant double values source, but the next question is whether it even needs to be added to the bindings then. Another cleanup discussed in #99667 is throwing UnsupportedOperationException from ReplaceableConstDoubleValueSource#explain as it should never be called. Implementing the method means we need to test it which makes little sense if the method is never called in production code.
Handling of _value in a script agg does not support search concurrency when using the expression script engine. The reason is that the value gets set globally assuming sequential execution. This commit addresses that by setting the value to the values source associated with the correct leaf reader context.
Closes #99156