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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
