You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "javanna (via GitHub)" <gi...@apache.org> on 2023/10/24 09:25:44 UTC
[PR] Make IndexSearcher#getSlices final and clarify docs [lucene]
javanna opened a new pull request, #12718:
URL: https://github.com/apache/lucene/pull/12718
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 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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make IndexSearcher#getSlices final and clarify docs [lucene]
Posted by "s1monw (via GitHub)" <gi...@apache.org>.
s1monw commented on code in PR #12718:
URL: https://github.com/apache/lucene/pull/12718#discussion_r1371505561
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -425,11 +425,12 @@ public int count(Query query) throws IOException {
}
/**
- * Returns the leaf slices used for concurrent searching
+ * Returns the leaf slices used for concurrent searching. Override {@link #slices(List)} to
+ * customize how slices are created.
*
* @lucene.experimental
*/
- public LeafSlice[] getSlices() {
+ public final LeafSlice[] getSlices() {
Review Comment:
++
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make IndexSearcher#getSlices final and clarify docs [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #12718:
URL: https://github.com/apache/lucene/pull/12718
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make IndexSearcher#getSlices final and clarify docs [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12718:
URL: https://github.com/apache/lucene/pull/12718#discussion_r1369871845
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -115,10 +115,10 @@ public class IndexSearcher {
protected final List<LeafReaderContext> leafContexts;
/**
- * used with executor - LeafSlice supplier where each slice holds a set of leafs executed within
+ * Used with executor - LeafSlice supplier where each slice holds a set of leafs executed within
* one thread. We are caching it instead of creating it eagerly to avoid calling a protected
- * 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.
Review Comment:
the behaviour here has recently changed, but the docs were out of date.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make IndexSearcher#getSlices final and clarify docs [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12718:
URL: https://github.com/apache/lucene/pull/12718#issuecomment-1780674911
thanks @s1monw for the review! I will add a changes entry and merge this.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make IndexSearcher#getSlices final and clarify docs [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12718:
URL: https://github.com/apache/lucene/pull/12718#discussion_r1369871488
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -425,11 +425,12 @@ public int count(Query query) throws IOException {
}
/**
- * Returns the leaf slices used for concurrent searching
+ * Returns the leaf slices used for concurrent searching. Override {@link #slices(List)} to
+ * customize how slices are created.
*
* @lucene.experimental
*/
- public LeafSlice[] getSlices() {
+ public final LeafSlice[] getSlices() {
Review Comment:
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.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org