Introduce TwoPhaseCollector to leverage ContextIndexSearcher in AggregatorTestCase #98835
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iverase
added
>non-issue
:Analytics/Aggregations
elasticsearchmachine
added
Team:Search
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Pinging @elastic/es-search (Team:Search) |
martijnvg
approved these changes
Aug 24, 2023
| } else if (aggsCollector instanceof InternalProfileCollector profileCollector) { | ||
| profileCollector.doPostCollection(); | ||
| if (aggsCollector instanceof TwoPhaseCollector twoPhaseCollector) { | ||
| twoPhaseCollector.doPostCollection(); |
There was a problem hiding this comment.
Should aggsCollector always be a TwoPhaseCollector? Since BucketCollectorWrapper and InternalProfileCollector implement that interface?
There was a problem hiding this comment.
Yes, but trying to exploit that was a headache because some tests are abusing the Collector interface so I am leaving this for a follow up
| @@ -181,6 +181,11 @@ public boolean supportsSampling() { | |||
| return true; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean supportsParallelCollection() { | |||
There was a problem hiding this comment.
I assume that this agg didn't support parallel collection also before this change?
There was a problem hiding this comment.
Indeed, the postcollection method is accessing doc values created in the coordinating thread.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In oder to leverage the call of postcollection in the ContextIndexSearcher, we need to introduce an abstraction that can be leverage by AggregatorTestCase. Therefore we introduce the TwoPhaseCollector which simplifies a bit the code and allow to create those collector in the AggregatorTestCase.
relates #98672