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

fix: Correct the order of generating default and overridden broker capacities in Capacity.processCapacityEntries #10509

Merged
merged 2 commits into from
Aug 30, 2024
Merged

fix: Correct the order of generating default and overridden broker capacities in Capacity.processCapacityEntries #10509

merged 2 commits into from
Aug 30, 2024

Conversation

Xander-run
Copy link
Contributor

Type of change

  • Bugfix

Description

Correct the order of generating default and overridden broker capacities in Capacity.processCapacityEntries. The order after the change will be:

  1. Initialize the broker capacities based on spec.cruiseControl.brokerCapacity.
  2. Override the default capacities base on spec.cruiseControl.brokerCapacity.overrides.

This reduces the unneeded loop when generating broker capacity configurations and will resolve issue #10465

Checklist

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

I ran and passed test class io.strimzi.operator.cluster.model.CruiseControlTest for this change.

@Xander-run Xander-run marked this pull request as ready for review August 27, 2024 21:13
@Xander-run
fix: Correct the order of generating default and overridden broker ca…
f1fe805 
…pacities in Capacity.processCapacityEntries

Signed-off-by: Tian Lu <[email protected]>
@scholzj scholzj added this to the 0.44.0 milestone Aug 27, 2024
scholzj
scholzj reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left one nit. But I think it looks good otherwise.

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/cruisecontrol/Capacity.java Outdated
// Override default capacities
if (brokerCapacity != null) {
// For checking for duplicate brokerIds
Set<Integer> overrideIds = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this was wrong already before. But maybe you can initialize this only later before the for loop on line 387? That way we won't do an unnecessary initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing

@scholzj scholzj linked an issue Aug 27, 2024 that may be closed by this pull request
[BUG] The validation of the override node IDs in the Capacity class results in invalid warnings #10465
Closed
kyguy
kyguy approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@kyguy kyguy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good @Xander-run, thanks for the fix!

The unit test failures appear to be unrelated to the code changes, could you try running the flagged tests locally to confirm?

@Xander-run
Copy link
Contributor Author

The unit test failures appear to be unrelated to the code changes, could you try running the flagged tests locally to confirm?

I'm not quite sure what you mean by "flagged tests". Is it the CruiseControl group of the system test?

@scholzj
Copy link
Member

scholzj commented Aug 28, 2024 •
edited
Loading

The unit test failures appear to be unrelated to the code changes, could you try running the flagged tests locally to confirm?

I'm not quite sure what you mean by "flagged tests". Is it the CruiseControl group of the system test?

@Xander-run There was a flaky test that failed originally. But I restarted the tests and it passed. You can just ignore it.

@scholzj
Copy link
Member

scholzj commented Aug 29, 2024

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

ppatierno
ppatierno approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

@scholzj scholzj merged commit 3224ce8 into strimzi:main Aug 30, 2024
21 checks passed
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

@kyguy kyguy kyguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.44.0
Development

Successfully merging this pull request may close these issues.

[BUG] The validation of the override node IDs in the Capacity class results in invalid warnings
4 participants
@Xander-run @scholzj @ppatierno @kyguy