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
Address concurrency issue in top hits aggregation #106990
Conversation
Hi @javanna, I've created a changelog YAML for you. |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
LGTM, thank you. I will close PR #106910 as a result of having this one. |
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
Would it be able to add a test to TopHitsAggregatorTests
that would otherwise fail? My understanding is that if stored fields or doc values fields are fetched via the top hits agg we could run into the concurrency issue that this PR tries to fix?
@martijnvg I already updated |
...er/src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregator.java
Outdated
Show resolved
Hide resolved
Thanks all for the reviews and help! |
Top hits aggregation runs the fetch phase concurrently when the query phase is executed across multiple slices. This is problematic as the fetch phase does not support concurrent execution yet. The core of the issue is that the search execution context is shared across slices, which call setLookupProviders against it concurrently, setting each time different instances of preloaded source and field lookup providers. This makes us cross streams between slices, and hit lucene assertions that ensure that stored fields loaded from a certain thread are not read from a different thread. We have not hit this before because the problem revolves around SearchLookup which is used by runtime fields. TopHitsIT is the main test we have for top hits agg, but it uses a mock script engine which bypasses painless and SearchLookup.
Top hits aggregation runs the fetch phase concurrently when the query phase is executed across multiple slices. This is problematic as the fetch phase does not support concurrent execution yet. The core of the issue is that the search execution context is shared across slices, which call setLookupProviders against it concurrently, setting each time different instances of preloaded source and field lookup providers. This makes us cross streams between slices, and hit lucene assertions that ensure that stored fields loaded from a certain thread are not read from a different thread. We have not hit this before because the problem revolves around SearchLookup which is used by runtime fields. TopHitsIT is the main test we have for top hits agg, but it uses a mock script engine which bypasses painless and SearchLookup.
Top hits aggregation runs the fetch phase concurrently when the query phase is executed across multiple slices. This is problematic as the fetch phase does not support concurrent execution yet.
The core of the issue is that the search execution context is shared across slices, which call
setLookupProviders
against it concurrently, setting each time different instances of preloaded source and field lookup providers. This makes us cross streams between slices, and hit lucene assertions that ensure that stored fields loaded from a certain thread are not read from a different thread.We have not hit this before because the problem revolves around
SearchLookup
which is used by runtime fields.TopHitsIT
is the main test we have for top hits agg, but it uses a mock script engine which bypasses painless andSearchLookup
.