Thanks for finding this Tomas! This is a pretty bad performance regression for certain cases (facet.method=enum should be hit pretty hard too I imagine, along with any code that un-inverts a field).

We should try to fix this for 5.3!

We should try to fix this for 5.3!

+1. I'm looking at SOLR-7875 first, but if after improving SolrQueryTimeoutImpl the performance is not still as good as the un-wrapped version I think we should have a way to skip the wrapping.

I tried implementing something that would use a different reader when a timeout isn't needed on a per-query basis, but I ran into issues
From TermQuery / TermWeight:

      assert termStates.topReaderContext == ReaderUtil.getTopLevelContext(context) : "The top-reader used to create Weight (" + termStates.topReaderContext + ") is not the same as the current reader's top-reader (" + ReaderUtil.getTopLevelContext(context);

createWeight takes a searcher, and the TermWeight implementation remembers the top reader context from that searcher, later comparing it against the readers used for scoring.

I got into the exact same problem, query must be rewritten with the same reader as the one used to search. Is it a good idea to chose different readers based on parameters for search? Seems like o.a.l.s.IndexSearcher is not designed to do it.

Only way I can think of to work around it is to use a thread local (like SolrRequestInfo) to return one reader or the other for searcher.getTopReaderContext(). I don't really like it, but it might be the only way.

Bulk move to 5.4 after 5.3 release.

Here's a generic patch that escapes the ExitableDirectoryReader wrapping as soon as possible (in the fields() call). It should work, but I haven't had a chance to test the performance.

Interesting. This patch fixes one of the new tests I made for SOLR-7990. TimeAllowedCachingTest.testQueryResults. Submitting a query that times out followed immediately by an identical one only with a bigger timeAllowed that does NOT time out used to still return partialResults=true but doesn't with this patch. Which makes some sense I think.

Hmmm, that is interesting, and it raises red flags for me as I'm not sure why this patch would affect caching. ExitableDirectoryReader and friends delegate cache core keys to the underlying readers. Any theories / clues?

The test that it cured was this sequence:

> send a query that times out (no fq clauses)
> send the same query that does not time out (still no fq clauses)
***>test the second response to see if partialResults==true

It's the *** part the suddenly succeeded after this patch. Prior to this patch partialResults was true, now it's just not returned in the second response at all, which makes sense on the surface if your patch bails out from that wrapper earlier. This test was returning the correct number of docs before, it was just that the partialResults was incorrectly returned.

The tests that go against the filterCache are still failing though, so it's not like this patch magically fixed any of the caching problems.

So I'd guess not returning the partialResults flag makes more sense in terms of this patch?

Haven't updated the test patch yet, just tried it locally.

OK, on a second look it was a bad test.

copy/pasted the original code for the first call with timeAllowed=1 that looked at partialResults but didn't test for null. Just saw the error and didn't read it carefully.

assertFalse("Should NOT have partial results", (Boolean) (header.get("partialResults"));

When timeAllowed=1000, get("partialResults") returns null.....

So this patch has no effect whatsoever on the new tests I wrote. I removed the linked-to.

OK, I did some ad-hoc performance testing on near worst case range query on id field and saw an 8% performance improvement on my box.
I'll plan on committing this shortly.

Commit 1700276 from Yonik Seeley in branch 'dev/trunk'
[ https://svn.apache.org/r1700276 ]

SOLR-7876: exit ExitableDirectoryReader wrapper if timeout is not enabled

Yonik Seeley, Any reason why this couldn't be backported?

The QueryTimeout interface was put into Lucene when this feature (SOLR-5986) was committed, and I changed it's signature.

Oh, I didn't notice you where changing the interface. Makes sense then.