Clarify IndexSearcher#setTimeout semantics

[javanna]

I read the discussion around the introduction of the IndexSearcher#setTimeout in #927 . From that conversation, it was suggested to introduce a global setTimeout method, instead of adding a timeout argument to the existing search method.

That design has an important consequence: users who want to take advantage of the timeout mechanism need to use one IndexSearcher implementation per query, more or less. QueryTimeoutImpl follows that same principle as it sets the expiration time in its constructor, meaning that when a search will time out is pre-determined at the time that setTimeout is called. There is an additional scenario where one would run multiple queries under the hood to power a single logical query, and would like to apply the same expiration time globally.

I think at the very least we should improve the javadocs around the above expectations to ensure that users understand the implications of using setTimeout.

In many applications the searcher is shared among different threads: setTimeout makes the timeout mutable without handling any concurrency which seems like a bug. I understand that this may come from the expectation that in order to use the timeout mechanism, the searcher would be used from a single thread, but that is not enforced anywhere. setTimeout can be called at anytime, and that may affect already running searches? In situations where a logical search is composed of multiple lucene search operations, these could be parallelized. I would not expect setTimeout to be called concurrently (although technically possible) but I thought that we should ensure that all threads see the same value for it?

I also wonder if there is a use-case for setting the timeout more than once to the same searcher, instance it feels like it should be set once and never updated?

All in all, I am wondering if we should make the queryTimeout member final and remove the setTimeout method. I feel like that'd make expectations clearer, and I'd like feedback to verify that that wouldn't block certain use-cases. An alternative would be to handle concurrency around accessing the mutable queryTimeout member.

[javanna]

I also noticed that the isPartial flag is set to true whenever a timeout happens but never set back to false. Does that come from the expectation that once a search has timed out no more searches would be executed using the same searcher? Users can read the value through the timedOut method and decide not to run further searches, but if they do go ahead, they won't be able to know if subsequent search operations have timed out or not after one has. How do we expect users to interact with the timedOut method?

[jpountz]

+1 to improve javadocs

This discussion also relates to #11700.

I am wondering if we should make the queryTimeout member final and remove the setTimeout method.

I would like it better too. In general, all the setters that IndexSearcher has are expected to be called right after calling the constructor, and to never be touched again, so they would be better implemented as final members. I worry this will end up make us implement a builder for IndexSearcher, but it's probably better than what we have today?