-
Notifications
You must be signed in to change notification settings - Fork 982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make IndexSearcher#getSlices final and clarify docs #12718
Conversation
IndexSearcher exposes a public getSlices method, that is used to retriefve the slices that the searcher executes queries against, as well as slices, which is supposed to be overridden to customize the creation of slices. I believe that getSlices should be final: there is no reason to override the method. Also, it is too easy to confuse the two and end up overriding the wrong one by mistake.
* | ||
* @lucene.experimental | ||
*/ | ||
public LeafSlice[] getSlices() { | ||
public final LeafSlice[] getSlices() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we are good backporting this to 9.x given that the method is experimental. I don't see a reason why one would override getSlices on purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
* method from constructor, which is a bad practice. This is {@code null} if no executor is | ||
* provided | ||
* method from constructor, which is a bad practice. Always non-null, regardless of whether an | ||
* executor is provided or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the behaviour here has recently changed, but the docs were out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @javanna!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks @s1monw for the review! I will add a changes entry and merge this. |
IndexSearcher exposes a public getSlices method, that is used to retrieve the slices that the searcher executes queries against, as well as slices, which is supposed to be overridden to customize the creation of slices. I believe that getSlices should be final: there is no reason to override the method. Also, it is too easy to confuse the two and end up overriding the wrong one by mistake.
IndexSearcher exposes a public getSlices method, that is used to retriefve the slices that the searcher executes queries against, as well as slices, which is supposed to be overridden to customize the creation of slices.
I believe that getSlices should be final: there is no reason to override the method. Also, it is too easy to confuse the two and end up overriding the wrong one by mistake.
This change was discussed here: #12606 (comment) .