zacharymorn commented on a change in pull request #240: URL: https://github.com/apache/lucene/pull/240#discussion_r692722475
########## File path: lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/ReadTask.java ########## @@ -180,6 +185,7 @@ protected int withTopDocs(IndexSearcher searcher, Query q, TopDocs hits) throws return res; } + @Deprecated Review comment: > I wonder if anyone out there has sub-classed this to provide their own Collector through overriding this method? It's possible right? Hmm you are right. I reverted some changes in this class to still use the `ReadTask#createCollector` method, in case there's overriding from users. > Would we want to support this with a CollectorManager now instead? You might consider creating a new protected method--createCollectorManager--that sub-classes could override if they want, then add javadoc here with a @lucene.deprecated tag pointing users to the new method. I think this `createCollector` method was originally added (11 years ago) to support benchmarking, and not sure how widely this is used actually? So hopefully marking it as deprecated for release 9.0 and removing it entirely once we release 10.0 should be good enough? ########## File path: lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java ########## @@ -527,7 +503,10 @@ public TopDocs search(Query query, int n) throws IOException { * * @throws TooManyClauses If a query would exceed {@link IndexSearcher#getMaxClauseCount()} * clauses. + * @deprecated This method is being deprecated in favor of {@link IndexSearcher#search(Query, Review comment: > I think we like the @lucene.deprecated tag? Do you mean `@lucene.deprecated` should be created and used instead of using `@deprecated`? I can't seems to find that tag is being used in lucene actually...I think `@lucene.deprecated` most likely won't be recognized by IDE or build tool to signal / warn the use of deprecated methods though. > Is your reasoning behind keeping this in there for now that all of our uses of this (internal to Lucene) haven't yet migrated as part of this change? Would the plan me to migrate all usages off of this internally and then actually remove it on main/9.0, or are you thinking of keeping it around until 10.0? I think our backwards compatibility policy is such that we could just directly remove this on main/9.0, but then leave it like you have it (marked deprecated) if you choose to backport this to 8x. Since this method is so fundamental though, I could easily see an argument to keep it around for an extra major release to give users more time to migrate. Then again, the migration path seems pretty straight-forward. What do you think? As explained in https://issues.apache.org/jira/browse/LUCENE-10002?focusedCommentId=17397827&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17397827, I use deprecation instead of direct removal in this PR exactly for the reasons you mentioned: 1. Not all internal uses have been migrated in this PR (which may require another few thousand lines of changes). 1. I personally feel that we should give users more time for migration and testing out the changes, so 10.0 release might be a good timing for complete removal. I would be interested in seeing how folks feel about the timing of removal, but after #1 is completed, complete removal of IndexSearcher#search(Query, Collector) in lucene codebase should require only straightforward deletion changes. -- 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