You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/07/01 23:20:37 UTC
[GitHub] [lucene] msokolov commented on a diff in pull request #996: LUCENE-10151: Some fixes to query timeouts.
msokolov commented on code in PR #996:
URL: https://github.com/apache/lucene/pull/996#discussion_r912285264
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -730,29 +737,25 @@ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector c
}
BulkScorer scorer = weight.bulkScorer(ctx);
if (scorer != null) {
- if (queryTimeout != null) {
- TimeLimitingBulkScorer timeLimitingBulkScorer =
- new TimeLimitingBulkScorer(scorer, queryTimeout);
- try {
- timeLimitingBulkScorer.score(leafCollector, ctx.reader().getLiveDocs());
- } catch (
- @SuppressWarnings("unused")
- TimeLimitingBulkScorer.TimeExceededException e) {
- partialResult = true;
- }
- } else {
- try {
- scorer.score(leafCollector, ctx.reader().getLiveDocs());
- } catch (
- @SuppressWarnings("unused")
- CollectionTerminatedException e) {
- // collection was terminated prematurely
- // continue with the following leaf
- }
+ if (queryTimeout != null && queryTimeout.isTimeoutEnabled()) {
Review Comment:
phew thanks for all the cleanup here. I guess we would not have been able to handle early termination and timeouts together... It's dangerous to commit things without better testing!
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -85,7 +85,11 @@ public class IndexSearcher {
private static QueryCache DEFAULT_QUERY_CACHE;
private static QueryCachingPolicy DEFAULT_CACHING_POLICY = new UsageTrackingQueryCachingPolicy();
private QueryTimeout queryTimeout = null;
- private boolean partialResult = false;
+ // TODO: does partialResult need to be volatile? It can be set on one of the threads of the
Review Comment:
I appreciate the comment here, but do we need to keep the TODO? Seems ok to simply make it volatile
##########
lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java:
##########
@@ -2033,6 +2034,15 @@ protected LeafSlice[] slices(List<LeafReaderContext> leaves) {
}
ret.setSimilarity(classEnvRule.similarity);
ret.setQueryCachingPolicy(MAYBE_CACHE_POLICY);
+ if (random().nextBoolean()) {
Review Comment:
oh, good idea to sometimes time out. It would be nice to vary the timeout behavior too and sometimes in fact time out? Although this would probably break many (even most) tests that do searches, so not sure what context it could apply to.
--
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