Remove CollectorManager#forSequentialExecution #13790
Conversation
We have recently (see apache#13735) introduced this utility method that creates a collector manager which only works when a searcher does not have an executor set, otherwise it throws exception once we attempt to create a new collector for more than one slice. While we discussed it should be safe to use in some specific scenarios like the monitor module, we should be careful exposing this utility publicly, because while we'd like to ease migration from the search(Query, Collector) method, we may end up making users like even worse, in that it exposes them to failures whenever an executor is set and there are more than one slice created, which is hard to follow and does not provide a good user experience. My proposal is that we use a similar collector manager locally, where safe and required, but we don't expose it to users. In most places, we should rather expose collector managers that do support search concurrency, rather than working around the lack of those.
|
@gsmiller how do you feel about this one? |
|
tl;dr: I agree with removing this. I was initially hesitant 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 |
|
Thanks for the feedback @gsmiller I will go ahead and merge this. |
We have recently (see #13735) introduced this utility method that creates a collector manager which only works when a searcher does not have an executor set, otherwise it throws exception once we attempt to create a new collector for more than one slice. While we discussed it should be safe to use in some specific scenarios like the monitor module, we should be careful exposing this utility publicly, because while we'd like to ease migration from the search(Query, Collector) method, we may end up making users like even worse, in that it exposes them to failures whenever an executor is set and there are more than one slice created, which is hard to follow and does not provide a good user experience. My proposal is that we use a similar collector manager locally, where safe and required, but we don't expose it to users. In most places, we should rather expose collector managers that do support search concurrency, rather than working around the lack of those.
We have recently (see #13735) introduced this utility method that creates a collector manager which only works when a searcher does not have an executor set, otherwise it throws exception once we attempt to create a new collector for more than one slice.
While we discussed it should be safe to use in some specific scenarios like the monitor module, we should be careful exposing this utility publicly, because while we'd like to ease migration from the search(Query, Collector) method, we may end up making users life even worse, in that it exposes them to failures whenever an executor is set and there is more than one slice created, which is hard to follow and does not provide a good user experience. What is these users do have an executor set to their searcher and think that this method provides a collector manager that is able to turn off concurrency (like search(Query, Collector) does currently), which is not possible?
My proposal is that we use a similar collector manager locally, where safe and required, but we don't expose it to users. In most places, we should rather expose collector managers that do support search concurrency, rather than working around the lack of those. Even for us committers, it has not been entirely clear when this is safe and when it's not, which indicates that this is something that users will likely struggle with.