Introduce AllGroupsCollectorManager - #15557
Conversation
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
@javanna, could you help to review this PR, thanks! |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
@jpountz @romseygeek @dweiss could you help to review this PR, thank you~ |
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
Hey @gaobinlong sorry for the delay in getting back to you. I had a look at this PR. I am not super familiar with the grouping API, so I may not be the best person to advise on how to move forward with the change. My initial thoughts: this PR introduces a new public API that is only used in a test. Can it be used by users as-is like such test does? Are there other places where Lucene itself could rely on this new collector manager in production code? it seems like All in all, I am trying to determine if this is a step forward in the right direction. It may make sense to play further with GroupingSearch or in alternative keep this change small but introduce the manager only to the test that it affects, unless we are 100% sure that Lucene users can use the new manager as-is. What do you think? |
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
Thanks for your comment, @javanna ,
Yes, I'm sure it can be used by users like the test cases.
Yes, I noticed that in GroupingSearch, and we need to replace the AllGroupsCollector with AllGroupsCollectorManager, but we need to also introduce FirstPassGroupCollectorManger and AllGroupHeadsCollectorManager, then use MultiCollectorManager to wrap all of the collectorManager into one, then it works.
No, this really is a problem, I've refactored the code to use T instead, thanks! |
javanna
left a comment
There was a problem hiding this comment.
Thanks for the update @gaobinlong . I had another look and left a couple more comments. I will merge this PR once those are addressed.
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
@javanna, sorry for the delay, I've addressed all of the new comments, please take another look, thanks! |
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
javanna
left a comment
There was a problem hiding this comment.
Left a comment about moving the changes entry, LGTM otherwise. Thanks!
|
|
||
| * GITHUB#15976: Store DocValuesSkipper data in their own file. (Alan Woodward) | ||
|
|
||
| * GITHUB#15557: Introduce AllGroupsCollectorManager to parallelize search when using AllGroupsCollector. (Binlong Gao) |
There was a problem hiding this comment.
Could you please move this to the 10.4 section? I think it makes sense to backport this change, for users to immediately start benefiting of the new collector manager.
There was a problem hiding this comment.
Done that, thanks!
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
AllGroupsCollector in grouping package did not have until now a corresponding collector manager, this pr introduces one and switches TestAllGroupsCollector to use search concurrency and move away from the deprecated search(Query, Collector) method. Relates to #12892 Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
Thanks @gaobinlong I merged this to main and backported to 10x. |
AllGroupsCollector in grouping package did not have until now a corresponding collector manager, this pr introduces one and switches TestAllGroupsCollector to use search concurrency and move away from the deprecated search(Query, Collector) method. Relates to apache#12892 Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Description
AllGroupsCollector in grouping package did not have until now a corresponding collector manager, this pr introduces one and switches TestAllGroupsCollector to use search concurrency and move away from the deprecated search(Query, Collector) method.
Relates to #12892.