zacharymorn commented on pull request #240: URL: https://github.com/apache/lucene/pull/240#issuecomment-921456717
> I left some comments but the approach that you took so far looks good to me. I see that we still need to migrate some collectors like LargeNumHitsTopDocsCollector or the internal JoinUtil collectors to a CollectorManager? Thanks @jpountz for the review! Indeed there are more that need to be migrated as well. In fact, I have been working on the 2nd PR to migrate more "easy" ones, and after that, we still have the following: #### Explanation related `ExplanationAsserter` `MatchesAsserter` #### Facet related `FacetsCollector` `RandomSamplingFacetsCollector` #### Grouping related `GroupFacetCollector` `TopGroupsCollector` `BlockGroupingCollector` `FirstPassGroupingCollector` `AllGroupHeadsCollector` `AllGroupsCollector` #### Global ordinal related `GlobalOrdinalsCollector` `GlobalOrdinalsWithScoreCollector` #### Anonymous classes Anonymous `Collector` used in `JoinUtil` Anonymous `Collector` used in `TestJoinUtil` Anonymous `Collector` used in `QueryUtils` #### Profiling related `ProfilerCollector` `MemoryAccountingBitsetCollector` `MonitorQueryCollector` #### Counting related `DocValuesStatsCollector` `MyHitCollector` `MatchCollector` #### Result diversification related `DistinctValuesCollector` `DiversifiedTopDocsCollector` #### Other `CachingCollector` `TerminateAfterCollector` `TimeLimitingCollector` `LargeNumHitsTopDocsCollector` As each of these categories / collectors will likely require more thoughts / time to understand the current logic, and come up with `CollectorManager` implementation that's also thread-safe, I'm thinking to create follow-up Jira sub-tickets for each of these categories to track them. What do you think? > I wonder if we should split by replacing the deprecation warnings of IndexSearcher#search(Query,Collector) and TopXXXCollector#create by some words about how the CollectorManager variants are recommended as they make it possible to search using multiple threads. Then when this PR is merged we can work more iteratively on replacing usage of Collector with CollectorManager, and we could add the deprecation warnings in the end once we have migrated all our code? Yeah I can definitely follow this strategy as well. The only concern I may have is that, as can be seen above, it may still take some time to migrate all existing collectors to collectorManagers. So using suggestion comment for now and adding deprecation warning at the end may likely see a few more collectors without collectorManagers added accidentally (i.e. folks may not notice the comment when they just code against the `IndexSearch#search(Query, Collector)` API)? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
