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-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
