strimzi-kafka-operator
strimzi-kafka-operator copied to clipboard
Extract Topic Operator's Cruise Control client
This is the first step in the direction of having a shared Cruise Control client based on Java HTTP client. For the moment changes are limited to the Topic Operator.
If we agree on the strategy, I can then move the Cruise Control client to operator-common and also use it in CruiseControlApi (different PR).
In my view, the shared Cruise Control client shouldn't be concerned with response handling. This client should simply send the request and return a response POJO. Much like Kafka admin and Kube clients do.
The response handling is specific to the functionality we are implementing (the same response could be processed in different ways), so it should be the responsibility of the caller (CruiseControlApi in CO, ReplicasChangeHandler in TO).
That way, each object in the call chain triggered by the controller is focused on doing one thing, and the code is easier to read and maintain.
This would leave a Vertx dependency in CruiseControlApi required by KafkaRebalanceAssemblyOperator. Of course we can also get rid of that, but I would leave it for a dedicated PR.
I see just moving a header to a dedicated class and nothing more in the direction of "extract a cruise control client" as the PR name suggests.
Here the long term objective is to have a shared CC thin client to be used by both cluster and topic operators. That was discussed in the RF change proposal. This is a first step in that direction, where I simply extract all CC client logic in the topic operator, so that I can later (in another PR) move it to operator-common and share with CruiseControlApi.
@scholzj @ppatierno thanks for the review.
This started as a simple refactoring, but I now agree on the necessity of a formal proposal before moving on with sharing the CC client with the cluster operator, and I will create one.
Current changes are mostly confined to the topic operator, so it would still be good to have them for better modularization.
I wonder if the shared CruiseControl client should really live in operator-common instead of a separate module or maybe even a seprate subproject.
That's an interesting question. A CruiseControl client could probably be useful in other contexts, so it may make sense to create a separate subproject.
Current changes are mostly confined to the topic operator, so it would still be good to have them for better modularization.
TBH, I left some comments, but I personally do not find this particular PR somehow controversial. So I do not have problem to proceed with this one right now. My comment was meant more in general.
/azp run regression
Azure Pipelines successfully started running 1 pipeline(s).
@tombentley are you ok with the current state? I would need this to complete some other TO changes.
Keep in mind that I'll create a new proposal for sharing the CC client with the CO where we can discuss the API in more details.
/azp run regression
Azure Pipelines successfully started running 1 pipeline(s).
@tombentley should be complete now, thanks for your suggestions.
/azp run regression
Azure Pipelines successfully started running 1 pipeline(s).