[GitHub] [lucene] javanna opened a new issue, #12275: Clarify IndexSearcher#setTimeout semantics

["javanna (via GitHub)" <gi...@apache.org>]

javanna opened a new issue, #12275:
URL: https://github.com/apache/lucene/issues/12275

   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.
   


-- 
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.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

[GitHub] [lucene] javanna commented on issue #12275: Clarify IndexSearcher#setTimeout semantics

["javanna (via GitHub)" <gi...@apache.org>]

javanna commented on issue #12275:
URL: https://github.com/apache/lucene/issues/12275#issuecomment-1538902519

   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?
   


-- 
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

[GitHub] [lucene] jpountz commented on issue #12275: Clarify IndexSearcher#setTimeout semantics

["jpountz (via GitHub)" <gi...@apache.org>]

jpountz commented on issue #12275:
URL: https://github.com/apache/lucene/issues/12275#issuecomment-1600503443

   +1 to improve javadocs
   
   This discussion also relates to https://github.com/apache/lucene/issues/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?


-- 
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