Skip to content
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.

Sign up for GitHub

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

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky #8963

Merged
merged 1 commit into from
Jul 28, 2023
Merged

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky #8963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,10 @@ private void searchLeaf(LeafReaderContext ctx, Weight weight, Collector collecto
}
}
}

// Note: this is called if collection ran successfully, including the above special cases of
// CollectionTerminatedException and TimeExceededException, but no other exception.
leafCollector.finish();
}

private Weight wrapWeight(Weight weight) {
Expand Down
11 changes: 9 additions & 2 deletions server/src/main/java/org/opensearch/search/suggest/completion/CompletionSuggester.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.lucene.search.BulkScorer;
import org.apache.lucene.search.CollectionTerminatedException;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.LeafCollector;
import org.apache.lucene.search.Weight;
import org.apache.lucene.search.suggest.document.CompletionQuery;
import org.apache.lucene.search.suggest.document.TopSuggestDocs;
Expand Down Expand Up @@ -108,15 +109,21 @@ private static void suggest(IndexSearcher searcher, CompletionQuery query, TopSu
for (LeafReaderContext context : searcher.getIndexReader().leaves()) {
BulkScorer scorer = weight.bulkScorer(context);
if (scorer != null) {
LeafCollector leafCollector = null;
Copy link
Collaborator

@nknize nknize Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized this fix is simpler.

Per this Query#rewrite deprecation just change line 106 to the following

                    query = (CompletionQuery) query.rewrite(searcher);

You're basically reimplementing here the finish hook that was added in Lucene PR#12380.

That logic path wasn't being executed though because we're using the old collector API through the creation of a new searcher wrapping the reader.

I forgot about this deprecation in the 9.7 upgrade and it wasn't exposed because #12380 wasn't merged until after 9.7.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about it, merged just before, there is another issue I run into, again with Suggester but slightly different cause:

java.lang.IllegalArgumentException: onlrtdbizi is not a SuggestField
	at org.apache.lucene.search.suggest.document.CompletionWeight.bulkScorer(CompletionWeight.java:80)
	at org.opensearch.search.suggest.completion.CompletionSuggester.suggest(CompletionSuggester.java:110)
	at org.opensearch.search.suggest.completion.CompletionSuggester.innerExecute(CompletionSuggester.java:78)
	at org.opensearch.search.suggest.completion.CompletionSuggester.innerExecute(CompletionSuggester.java:1)

I could incorporate your suggestion into the upcoming fix (still trying to figure out what is happening)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finishing a PR that cuts over all rewrites to the non-deprecated one. It includes this change in the CompletionSuggester and other places.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened the PR but I left your patch in. I was wrong, there's still an issue and cutting over to invoke the hook doesn't solve the occasional failure. Let me know if that's what you're investigating.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened the PR but I left your patch in. I was wrong, there's still an issue and cutting over to invoke the hook doesn't solve the occasional failure.

Do you have a sample stack trace or logs by any chance?

Let me know if that's what you're investigating.

The one that caused another batch of flakyness: #8968

try {
scorer.score(collector.getLeafCollector(context), context.reader().getLiveDocs());
leafCollector = collector.getLeafCollector(context);
scorer.score(leafCollector, context.reader().getLiveDocs());
} catch (CollectionTerminatedException e) {
// collection was terminated prematurely
// continue with the following leaf
}
// Note: this is called if collection ran successfully, including the above special cases of
// CollectionTerminatedException and TimeExceededException, but no other exception.
if (leafCollector != null) {
leafCollector.finish();
}
}
}
collector.finish();
}

@Override
Expand Down