-
Notifications
You must be signed in to change notification settings - Fork 590
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
base: migrate_to_kafka_2_4
Are you sure you want to change the base?
Make BrokerSetAwareGoal configurable to accept partition coloring #1864
Conversation
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.
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) { |
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.
Instead of doing this i would recommend introducing inheritance.
- Introduce a configuration -
broker.set.awareness.level=partition
We can have only two choices -partition
andtopic
- The Level can be picked from an ENUM so that no other level is allowed.
- The default value of this configuration can be set to level
topic
. - Introduce an interface called
BrokerSetAwarenessValidator
- Implement two implementations
TopicLevelBrokerSetAwarenessValidator
andPartitionLevelBrokerSetAwarenessValidator
- Based on configuration set, the instance of the BrokerSetAwarenessValidator will be chosen.
- From here just call
brokerSetAwarenessValidator.validate(partitions)
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.
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(); |
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 would not use word coloring
since it may be confusing. We have been referring to it as brokerserAwareness
This PR resolves #1862 .
As in the issue described, there are 2 solutions:
@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.