gsmiller commented on PR #13790: URL: https://github.com/apache/lucene/pull/13790#issuecomment-2354128424
tl;dr: I agree with removing this. I was [initially hesitant](https://github.com/apache/lucene/pull/13735#issuecomment-2338340094) to add this for a lot of the same reasons, but was convinced when realizing we could at least explicitly fail by asserting that more than one collector is never created (so at least we wouldn't be silently creating tear-your-hair-out concurrency bugs). But looking at the bigger picture, the whole point of removing `IndexSearcher#search(Query, Collector)` is to help users avoid traps situations where they think they're setup for concurrent search but not utilizing it. I agree it's a worse trap in a lot of ways to introduce this failure mode. I particularly do not like that this "helper" collector manager can't _actually_ check if it's being used with a concurrency-ready IndexSearcher. So I can imagine a real failure case where IndexSearchers are being created with an Executor but only have a single segment and things appear to workâbut then break later (e.g., users that are force-mergi ng their segments, or users that have sparse test coverage that only includes testing over single segments, etc.). -- 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
