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.

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

Make BrokerSetAwareGoal configurable to accept partition coloring #1864

Open
wants to merge 1 commit into
base: migrate_to_kafka_2_4
Choose a base branch
from

Conversation

CCisGG
Copy link
Contributor

@CCisGG CCisGG commented Jul 15, 2022

This PR resolves #1862 .

As in the issue described, there are 2 solutions:

  1. Make this goal easily extendable, and users can create a new goal reusing the code of this goal
  2. Make this goal configurable.

@efeg originally suggests that we'd better have a new goal, but after consideration, I think make it configurable is a cleaner solution. With this new configuration "allow.partition.level.broker.set.aware", only minimal code changes are needed.

@efeg Please feel free to comment on any benefits of creating a new goal if you think that's better.

@CCisGG CCisGG requested a review from efeg July 15, 2022 18:15
@CCisGG CCisGG self-assigned this Jul 15, 2022
Copy link
Contributor

@mohitpali mohitpali left a comment

Choose a reason for hiding this comment

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

Did an initial quick pass. I will give it another go again.

if (_brokersByBrokerSet.values().stream().noneMatch(brokerSetBrokers -> brokerSetBrokers.containsAll(allBrokersForTopic))) {
throw new OptimizationFailureException(
String.format("[%s] Topic %s is not brokerSet-aware. brokers (%s).", name(), topicName, allBrokersForTopic));
if (_isPartitionLevelBrokerSetAware) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this i would recommend introducing inheritance.

  1. Introduce a configuration - broker.set.awareness.level=partition
    We can have only two choices - partition and topic
  2. The Level can be picked from an ENUM so that no other level is allowed.
  3. The default value of this configuration can be set to level topic.
  4. Introduce an interface called BrokerSetAwarenessValidator
  5. Implement two implementations TopicLevelBrokerSetAwarenessValidator and PartitionLevelBrokerSetAwarenessValidator
  6. Based on configuration set, the instance of the BrokerSetAwarenessValidator will be chosen.
  7. From here just call brokerSetAwarenessValidator.validate(partitions)

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also just introduce the validate() method in th ENUMs (with values PARTITION and TOPIC )

    private enum BrokerSetAwarenessLevel {
        TOPIC {
            public void validate(Partitions p) {
                // do something
            }
        },
        PARTITION {
            public void validate(Partitions p) {
                // do something different
            }
        }

@@ -123,6 +125,8 @@ public boolean isHardGoal() {
*/
@Override
protected void initGoalState(ClusterModel clusterModel, OptimizationOptions optimizationOptions) throws OptimizationFailureException {
_isPartitionLevelBrokerSetAware = _balancingConstraint.allowPartitionColoring();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use word coloring since it may be confusing. We have been referring to it as brokerserAwareness

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make BrokerSetAwareGoal capable of doing partition coloring
2 participants