Skip to content
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

Allow aggs to disable offloading sequential collection #98276

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

javanna
Copy link
Member

@javanna javanna commented Aug 8, 2023

In #98204 we are introducing unconditional offloading of collection to a separate thread pool, even for requests of phases that don't enable search concurrency. It turns out that some aggs don't support offloading their collection to a separate thread, as their postCollect method is executed on the search thread which trips a lucene assertion around reusing data structures pulled from the search worker thread.

With this commit we allow aggs to specify when they don't support offloading their sequential collection. Such aggs are a subset of the ones that already declare that they don't support concurrency entirely.

Relates to #96023

In elastic#98204 we are introducing unconditional offloading of collection to a
separate thread pool, even for requests of phases that don't enable
search concurrency. It turns out that some aggs don't support offloading
their collection to a separate thread, as their postCollect method is
executed on the search thread which trips a lucene assertion around
reusing data structures pulled from the search worker thread.

With this commit we allow aggs to specify when they don't support
offloading their sequential collection. Such aggs are a subset of the
ones that already declare that they don't support concurrency entirely.

Relates to elastic#96023
@javanna javanna added :Analytics/Aggregations Aggregations :Search/Search Search-related issues that do not fall into other categories >refactoring v8.10.0 labels Aug 8, 2023
@javanna javanna requested a review from martijnvg August 8, 2023 09:22
@elasticsearchmachine elasticsearchmachine added Team:Search Meta label for search team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Aug 8, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for now to me in order to progress #98204.
But I do think that we should look into executing postCollection on the search worker thread rather sooner than later.

@javanna javanna merged commit 36d60e2 into elastic:main Aug 8, 2023
@javanna javanna deleted the enhancement/aggs_offload_sequential branch August 8, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >refactoring :Search/Search Search-related issues that do not fall into other categories Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Meta label for search team v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants