-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract Topic Operator's Cruise Control client #10092
Conversation
24a4842
to
abd22af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment but not sure the impact it has on the current CC client. I mean, 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. Maybe it's just a not that great title and PR description which makes me hard the purpose?
...ommon/src/main/java/io/strimzi/operator/common/model/cruisecontrol/CruiseControlHeaders.java
Outdated
Show resolved
Hide resolved
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. |
454b9f5
to
f226884
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some nits. I do not see this PR partiularly controversial. But I wonder if the overall idea should have some higher level discussion or proposal:
- While doing smaller steps is good, it is hard to review the PRs when not understanding the end state
- It seems it could clarify what and how should such client work and what should it do
- I wonder if the shared CruiseControl client should really live in
operator-common
instead of a separate module or maybe even a seprate subproject.
...ommon/src/main/java/io/strimzi/operator/common/model/cruisecontrol/CruiseControlHeaders.java
Outdated
Show resolved
Hide resolved
.../main/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/CruiseControlApi.java
Show resolved
Hide resolved
@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.
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Should this be also reviewed by @tombentley as the TO SME?
...ommon/src/main/java/io/strimzi/operator/common/model/cruisecontrol/CruiseControlHeaders.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Approving because it doesn't have any big impact on what we have as CC client within the cluster operator. But to move to a new one, we should discuss everything in a proposal.
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
topic-operator/src/main/java/io/strimzi/operator/topic/ReplicasChangeHandler.java
Outdated
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/ReplicasChangeHandler.java
Outdated
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Outdated
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Outdated
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/UrlBuilder.java
Outdated
Show resolved
Hide resolved
@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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fvaleri, this is looking pretty good, but I had a few more questions. Thanks!
topic-operator/src/main/java/io/strimzi/operator/topic/ReplicasChangeHandler.java
Outdated
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Outdated
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Show resolved
Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java
Outdated
Show resolved
Hide resolved
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. Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
@tombentley should be complete now, thanks for your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fvaleri
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
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.