Refactor CC capacity configuration classes

[kyguy]

Type of change

• Refactoring

Description

Refactors Cruise Control capacity configuration classes.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• 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

[scholzj]

Some more small nits, but looks mostly good.

[scholzj]

One more nit. But LGTM otherwise.

One thing I do not want to be strict about, but maybe you can consider ... your code is very dense. Using some mepty lines on the right places can help the readability. Especially empty lines between some general statement and if and between end of if and the next statement. For example instead of:

        Map<Integer, BrokerCapacityOverride> overrideMap = new HashMap<>();
        if (brokerCapacity != null && brokerCapacity.getOverrides() != null && !brokerCapacity.getOverrides().isEmpty()) {
            for (BrokerCapacityOverride override : brokerCapacity.getOverrides()) {
                List<Integer> ids = override.getBrokers();
                for (int id : ids) {
                    if (overrideMap.containsKey(id)) {
                        LOGGER.warnCr(reconciliation, "Duplicate broker id {} found in overrides, using first occurrence.", id);
                    } else if (kafkaBrokerNodes.stream().noneMatch(node -> node.nodeId() == id)) {
                        LOGGER.warnCr(reconciliation, "Ignoring broker capacity override for unknown node ID {}", id);
                    } else {
                        overrideMap.put(id, override);
                    }
                }
            }
        }
        return overrideMap;

you could do:

        Map<Integer, BrokerCapacityOverride> overrideMap = new HashMap<>();

        if (brokerCapacity != null && brokerCapacity.getOverrides() != null && !brokerCapacity.getOverrides().isEmpty()) {
            for (BrokerCapacityOverride override : brokerCapacity.getOverrides()) {
                List<Integer> ids = override.getBrokers();

                for (int id : ids) {
                    if (overrideMap.containsKey(id)) {
                        LOGGER.warnCr(reconciliation, "Duplicate broker id {} found in overrides, using first occurrence.", id);
                    } else if (kafkaBrokerNodes.stream().noneMatch(node -> node.nodeId() == id)) {
                        LOGGER.warnCr(reconciliation, "Ignoring broker capacity override for unknown node ID {}", id);
                    } else {
                        overrideMap.put(id, override);
                    }
                }
            }
        }

        return overrideMap;

To better visually split the different parts of the code. But obviously, this is pretty subjective, so feel free to ignore it.

[scholzj]

/azp run regression

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[katheris]

Had a couple of small comments/suggestions but overall looks good

[ppatierno]

LGTM. Great work!

[ppatierno]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kyguy]

There were a few flakes in the regression tests. I’ve gone ahead and squashed and rebased the code just in case. Once the new build and unit tests pass, we can rerun the regression tests to see if the issue persists.

[ppatierno]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[scholzj]

Thanks for the PR @kyguy