bruno-roustant commented on a change in pull request #1998:
URL: https://github.com/apache/lucene-solr/pull/1998#discussion_r509142656
##########
File path:
lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
##########
@@ -496,35 +496,38 @@ public TermsEnum iterator() throws IOException {
* exitable enumeration of terms.
*/
public static class ExitableTermsEnum extends FilterTermsEnum {
-
+ private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 4) -
1; // 15
Review comment:
Can you rename "MAX"? I propose "TIMEOUT_CHECK_SAMPLING" or
"NUM_CALLS_PER_TIMEOUT_CHECK".
##########
File path:
lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
##########
@@ -496,35 +496,38 @@ public TermsEnum iterator() throws IOException {
* exitable enumeration of terms.
*/
public static class ExitableTermsEnum extends FilterTermsEnum {
-
+ private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 4) -
1; // 15
+ private int calls;
private QueryTimeout queryTimeout;
/** Constructor **/
public ExitableTermsEnum(TermsEnum termsEnum, QueryTimeout queryTimeout) {
super(termsEnum);
this.queryTimeout = queryTimeout;
- checkAndThrow();
+ checkAndThrowWithSampling();
}
/**
* Throws {@link ExitingReaderException} if {@link
QueryTimeout#shouldExit()} returns true,
* or if {@link Thread#interrupted()} returns true.
*/
- private void checkAndThrow() {
- if (queryTimeout.shouldExit()) {
- throw new ExitingReaderException("The request took too long to iterate
over terms. Timeout: "
- + queryTimeout.toString()
- + ", TermsEnum=" + in
- );
- } else if (Thread.interrupted()) {
- throw new ExitingReaderException("Interrupted while iterating over
terms. TermsEnum=" + in);
+ private void checkAndThrowWithSampling() {
Review comment:
I didn't find "checkAndThrow()" very clear. Since it is about timeout
check I propose to rename to "checkTimeoutWithSampling()"?
##########
File path:
lucene/core/src/test/org/apache/lucene/index/TestExitableDirectoryReader.java
##########
@@ -163,6 +163,17 @@ public void testExitableFilterTermsIndexReader() throws
Exception {
searcher.search(query, 10);
reader.close();
+ // Set a fairly high timeout value (infinite) and expect the query to
complete in that time frame.
+ // Not checking the validity of the result, but checking the sampling
kicks in to reduce the number of timeout check
+ CountingQueryTimeout queryTimeout = new CountingQueryTimeout();
+ directoryReader = DirectoryReader.open(directory);
+ exitableDirectoryReader = new ExitableDirectoryReader(directoryReader,
queryTimeout);
+ reader = new TestReader(getOnlyLeafReader(exitableDirectoryReader));
+ searcher = new IndexSearcher(reader);
+ searcher.search(query, 10);
+ reader.close();
+ assertEquals(3, queryTimeout.getShouldExitCallCount());
Review comment:
Here we compare to the expected call count 3. Because 3 TermsEnum are
created: it is a PrefixQuery and there is one TermsEnum created for
AutomatonQuery.intersect() (the next call timeout check is skipped once), then
2 TermsEnum created for the 2 matching terms "one" and "ones").
Would it be clearer to have a separate test method?
We could index more docs, for example 50 with a prefix-generated term (e.g.
"term"+increment).
There would be a PrefixQuery for "term", the same test code, and we would
test the TIMEOUT_CHECK_SAMPLING:
- 1 TermsEnum for Automaton.intersect(), and the next calls would be sampled
(50/TIMEOUT_CHECK_SAMPLING=3) => call count +4
- 1 TermsEnum for each enumerated term => call count +50
We could verify call count = 54 (instead of 101 without sampling)
##########
File path:
lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java
##########
@@ -496,35 +496,38 @@ public TermsEnum iterator() throws IOException {
* exitable enumeration of terms.
*/
public static class ExitableTermsEnum extends FilterTermsEnum {
-
+ private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 4) -
1; // 15
Review comment:
Also add a comment to clarify that this value must be a power of two
(due to the efficient bit mask check below).
----------------------------------------------------------------
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.
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
