There is a ConfigSetsHandler because it operates on a separate end point – it doesn't make sense to send ConfigSet-related commands to /admin/collections, it makes more sense from the end user perspective to send ConfigSet-related commands to /admin/configs. Given separate end points, separate handlers also make sense.
Okay, that is reasonable.
The point I am trying to make is that how the Overseer processes ConfigSet operations is an implementation detail of the Overseer. The ConfigSetHandler (which is not part of the Overseer) just needs an interface in order to tell the Overseer that it wants a ConfigSet operation processed, it shouldn't be concerned with the implementation details.
In my mind, it makes the opposite impression. The fact that we have a ZkController.getOverseerCollectionQueue and a different ZkController.getOverseerConfigSetQueue() suggests that the two queues are different but they are not, at least not yet. So why does this feature try to suggest that their implementation might be different at all? These things are easily changed so lets refactor it when we actually need to have different queues.
Right now we happen to use the same queue under the covers, but maybe we find in the future that's a bad idea (e.g. users have different QoS expectations between ConfigSet and Collection operations and we add ConfigSet operations that are long lived and block important Collection operations). If the interface between the Overseer and the ConfigSet handler doesn't refer to collections, we don't need to change anything outside of the Overseer if we change the processing in the future.
There are already different QoS expectations within the existing operations. For example, operations for different collections never block each other and operations such as cluster status, overseer status and request status never block. However, they are all managed by the same overseer and it can continue to be the case. Yes, what operations block what is not formally defined or enforced, which is something that can use some love.
The only reason why it is called a OverseerCollectionQueue is to indicate that it is the queue for OverseerCollectionProcessor.
That can be indicated with a variable/method name, not the type name.
Sure it could be if it is a generic queue but it is only used for the overseer collection processor and I don't see the need for another queue in Solr right now.
I don't wish to make the API/tasks pluggable so I understand your concern. That being said, there is a middle ground between API/tasks being pluggable and putting everything in the collection processor. All I'm arguing for is clean interfaces. Take
SOLR-7855 as an example; because we had Overseer/role related commands in the collection processor it made refactoring much more difficult. Doing what you suggest in my opinion would have the same effect
I understand what you are saying. I did the same for Overseer in
SOLR-6554 which grouped all related operations and moved them into their own classes (ClusterStateMutator, SliceMutator etc). In fact, I'd argue that SOLR-7855 didn't go far enough – it'd be great to have individual operations completely separate from the message processor such that they can be easily unit tested. I am very much on board with that.
I'm just a bit confused why we have an interface if we have just one implementation (YAGNI!) e.g. OverseerMessageHandler and OverseerMessageHandlerSelector. Similarily, OverseerCollectionProcessor doesn't add much over OverseerProcessor except for the static getOverseerMessageHandlerSelector method.
I don't think the dispatching here is complex and it is completely contained in the Overseer. After
SOLR-7855 we have an OverseerCollectionMessageHandler to handle overseer collection messages. If you are suggesting throwing the ConfigSet related commands in there (from OverseerConfigSetMessageHandler), you've just moved the dispatch code somewhere else.
I was referring to the OverseerMessageHandlerSelector actually. I assumed that you foresee more than one implementation in the future which would make the dispatching more complex, hence the comment. So to dispatch a request, at level 1, you have the OverseerMessageHandlerSelector and at level 2, you have an OverseerMessageHandler and at level 3, you have the processMessage inside the OverseerMessageHandler which sends the request to the right handler method. This is the complexity that I was referring to. Perhaps, we can get rid of OverseerMessageHandlerSelector?
Sorry for the vague comment earlier. I don't want to block you anymore than I already have. We can always refactor this in future. Thank you for cleaning up the mess in OverseerCollectionProcessor!