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 2025/02/21 14:35:55 UTC
[PR] Stop cloning index input when loading NRTSuggester [lucene]
javanna opened a new pull request, #14271:
URL: https://github.com/apache/lucene/pull/14271
Cloning the index input is not necessary when loading the NRTSuggester. Also, it causes a bug as the clone gets closed right before the suggester instance gets returned, which makes it unusable in the later lookup method which slices the index input while it's already closed.
This was not discovered so far due to a testing gap that is fixed by #14270
--
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] Stop closing index input when loading NRTSuggester [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #14271:
URL: https://github.com/apache/lucene/pull/14271
--
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] Stop cloning index input when loading NRTSuggester [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #14271:
URL: https://github.com/apache/lucene/pull/14271#discussion_r1965729900
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionsTermsReader.java:
##########
@@ -72,10 +72,8 @@ public final class CompletionsTermsReader implements Accountable {
*/
public synchronized NRTSuggester suggester() throws IOException {
if (suggester == null) {
- try (IndexInput dictClone = dictIn.clone()) { // let multiple fields load concurrently
- dictClone.seek(offset);
- suggester = NRTSuggester.load(dictClone, fstLoadMode);
- }
+ IndexInput indexInput = dictIn.slice("NRTSuggester", offset, dictIn.length() - offset);
Review Comment:
This got me confused wrt the PR title since slices are technically clones. Maybe update the title to say "stop closing the clone" instead of "stop cloning"?
--
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] Stop cloning index input when loading NRTSuggester [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #14271:
URL: https://github.com/apache/lucene/pull/14271#discussion_r1965909207
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionsTermsReader.java:
##########
@@ -72,10 +72,8 @@ public final class CompletionsTermsReader implements Accountable {
*/
public synchronized NRTSuggester suggester() throws IOException {
if (suggester == null) {
- try (IndexInput dictClone = dictIn.clone()) { // let multiple fields load concurrently
- dictClone.seek(offset);
- suggester = NRTSuggester.load(dictClone, fstLoadMode);
- }
+ IndexInput indexInput = dictIn.slice("NRTSuggester", offset, dictIn.length() - offset);
Review Comment:
Technically, we no longer call clone, that is what I meant :) but I see your point that we call it indirectly now.
--
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] Stop closing index input when loading NRTSuggester [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #14271:
URL: https://github.com/apache/lucene/pull/14271#issuecomment-2675212515
Thanks all!
--
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