kaivalnp commented on PR #13202: URL: https://github.com/apache/lucene/pull/13202#issuecomment-2017007823
Thanks for the review @vigyasharma! > Apart from those, one divergent behavior is that we won't be raising some form of `TimeExceededException` when we timeout and terminate the search. Wonder if we should have a way to surface that this was a timeout, v/s all the results we could collect within `visitLimit()` node traversal. I think this exception will be raised [here](https://github.com/apache/lucene/blob/04f335ad79cae019d1a552e41e0029d061d8e3d9/lucene/core/src/java/org/apache/lucene/search/TimeLimitingBulkScorer.java#L71-L84) automatically, because the timeout has already occurred? Though we won't know / raise this _explicitly because graph search timed out_, but the end goal of marking results as "incomplete" will still be satisfied? I'm also not sure how we'd do it explicitly, perhaps we require lower-level API changes to allow rewrites to time out more gracefully? > Could we add headers for each column with these benchmark results please? Oops, I missed adding the benchmark description and row headers earlier, updated now.. > Separately, should we deprecate `TimeLimitingCollector` ? It doesn't use `QueryTimeout` and I don't think we're using it anywhere. +1, I only [see](https://github.com/search?q=repo%3Aapache%2Flucene%20TimeLimitingCollector&type=code) occurrences of it in comments. I'll try to create a PR for this soon -- 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
