strimzi-kafka-operator icon indicating copy to clipboard operation
strimzi-kafka-operator copied to clipboard

Extract Topic Operator's Cruise Control client

Open fvaleri opened this issue 9 months ago • 9 comments

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.

fvaleri avatar May 09 '24 16:05 fvaleri

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.

fvaleri avatar May 13 '24 07:05 fvaleri

@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.

fvaleri avatar May 15 '24 09:05 fvaleri

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.

scholzj avatar May 15 '24 09:05 scholzj

/azp run regression

scholzj avatar May 15 '24 14:05 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar May 15 '24 14:05 azure-pipelines[bot]

@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.

fvaleri avatar May 19 '24 08:05 fvaleri

/azp run regression

scholzj avatar May 19 '24 12:05 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar May 19 '24 12:05 azure-pipelines[bot]

@tombentley should be complete now, thanks for your suggestions.

fvaleri avatar May 20 '24 09:05 fvaleri

/azp run regression

scholzj avatar May 22 '24 23:05 scholzj

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar May 22 '24 23:05 azure-pipelines[bot]