Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve Cruise Control client creation and testing #10918

Merged
merged 11 commits into from
Jan 30, 2025
Merged

Improve Cruise Control client creation and testing #10918

merged 11 commits into from
Jan 30, 2025

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Dec 6, 2024 •
edited
Loading

Type of change

  • Refactoring

Description

This patch improves Cruise Control client creation and testing.
It also removes the Kafka admin client creation method.

Closes #10916.

Checklist

  • Make sure all tests pass
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally

@fvaleri fvaleri requested a review from scholzj December 6, 2024 12:26
@ppatierno ppatierno added this to the 0.46.0 milestone Dec 6, 2024
@fvaleri fvaleri force-pushed the to-utils branch 2 times, most recently from 7054e5d to c1b176f Compare December 7, 2024 13:18
@fvaleri fvaleri requested a review from scholzj December 10, 2024 11:35
@ppatierno ppatierno requested a review from a team January 7, 2025 12:15
fvaleri added a commit to fvaleri/strimzi-kafka-operator that referenced this pull request Jan 7, 2025
@fvaleri
Make TopicOperatorConfig class consistent with the other operators
ee2777c 
This change makes the TopicOperatorConfig class consistent with the other operators.

For more info see: strimzi#10918 (comment)

Signed-off-by: Federico Valeri <[email protected]>
fvaleri added a commit to fvaleri/strimzi-kafka-operator that referenced this pull request Jan 7, 2025
@fvaleri
Make TopicOperatorConfig consistent with the other operators
c5df78a 
This change makes the TopicOperatorConfig class consistent with the other operators.

For more info see: strimzi#10918 (comment)

Signed-off-by: Federico Valeri <[email protected]>
fvaleri added a commit to fvaleri/strimzi-kafka-operator that referenced this pull request Jan 7, 2025
@fvaleri
Make TopicOperatorConfig consistent with the other operators
e0b260f 
This change makes the TopicOperatorConfig class consistent with the other operators.

For more info see: strimzi#10918 (comment)

Signed-off-by: Federico Valeri <[email protected]>
fvaleri added a commit to fvaleri/strimzi-kafka-operator that referenced this pull request Jan 7, 2025
@fvaleri
Make TopicOperatorConfig consistent with the other operators
16477b5 
This change makes the TopicOperatorConfig class consistent with the other operators.

For more info see: strimzi#10918 (comment)

Signed-off-by: Federico Valeri <[email protected]>
@fvaleri fvaleri mentioned this pull request Jan 7, 2025
Merged
2 tasks
katheris
katheris reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@katheris katheris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment around the new Config record, but I also think this PR needs to be renamed as "Remove unnecessary methods from TopicOperatorUtil class" seems misleading to me. I think something like "Refactor TopicOperatorConfig initialisation" would better describe the changes.

topic-operator/src/main/java/io/strimzi/operator/topic/cruisecontrol/CruiseControlClient.java Outdated Show resolved Hide resolved
@fvaleri fvaleri requested review from katheris and ppatierno January 17, 2025 13:15
@fvaleri
Copy link
Contributor Author

fvaleri commented Jan 23, 2025

@ppatierno @katheris I did what you asked in the community call.

katheris
katheris reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@katheris katheris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fvaleri, two small comments. Also I wonder if we could update the title to be more descriptive e.g. Remove client creation methods from TopicOperatorUtil class and add CC client config validation

topic-operator/src/main/java/io/strimzi/operator/topic/TopicOperatorConfig.java Outdated Show resolved Hide resolved
topic-operator/src/main/java/io/strimzi/operator/topic/TopicOperatorUtil.java Outdated Show resolved Hide resolved
@fvaleri fvaleri changed the title Remove unnecessary methods from TopicOperatorUtil class Remove client creation methods from TopicOperatorUtil class and add CC client config validation Jan 27, 2025
@fvaleri fvaleri changed the title Remove client creation methods from TopicOperatorUtil class and add CC client config validation Improve Cruise Control client creation and testing Jan 27, 2025
@fvaleri
Improve Cruise Control client creation and testing
ef58949 
This patch improves Cruise Control client creation and testing.
It also removes the Kafka admin client creation method.

Closes strimzi#10916.

Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Address comments
026098f 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Add CC client config validation
5156bda 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Improve create factory method
c620255 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Improve cruiseControlClientConfig
7c00507 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Remove conflict tag
99558a2 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Trigger build
24c5dcc 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Address feedback
4e7528a 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Revert Config change :(
bcab341 
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Address feedback
e5ff005 
Signed-off-by: Federico Valeri <[email protected]>
@scholzj
Copy link
Member

scholzj commented Jan 27, 2025

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fvaleri
Address feedback
49cb8a2 
Signed-off-by: Federico Valeri <[email protected]>
@scholzj scholzj merged commit bb37c15 into strimzi:main Jan 30, 2025
13 checks passed
@scholzj
Copy link
Member

scholzj commented Jan 30, 2025

Thanks for the PR.

@fvaleri fvaleri deleted the to-utils branch January 30, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj approved these changes

@ppatierno ppatierno ppatierno approved these changes

@katheris katheris katheris approved these changes

Assignees
No one assigned
Labels
needs review topic operator
Projects
None yet
Milestone
0.46.0
Development

Successfully merging this pull request may close these issues.

Remove unnecessary methods from TopicOperatorUtil class
4 participants
@fvaleri @scholzj @ppatierno @katheris