Add BrokerSetAwareGoal

[mohitpali]

This PR resolves #1782

Currently, cruise-control does not have a way to limit the leader and replica movement plan within a Broker Group. We would like to make cruise-control aware of the Broker Groups and propose optimizations where each optimization does leader movement or replica movement within a Broker Group. In short, each optimization should be limited within the boundary of Broker Group.

In this PR :

1. I have added new BrokerSetAwareGoal (Hard Goal).
2. I have also added a BrokerSetDataStore interface.
3. I have provided a file based default implementation to fetch BrokerSet Information called BrokerSetFileDataStore

[efeg]

Thanks for the PR @mohitpali -- quick note, looks like there are some JavaDoc formatting issues -- i.e.

> Task :cruise-control:javadoc
Picked up _JAVA_OPTIONS: -Xms512m -Xmx1g
/home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/goals/BrokerSetAwareGoal.java:60: error: unexpected end tag: </p>
 * </p>
   ^
/home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/goals/BrokerSetAwareGoal.java:68: error: bad use of '>'
 * _excludedTopics -> Handled by BrokerSetAwareGoal
                    ^
/home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/goals/BrokerSetAwareGoal.java:69: error: bad use of '>'
 * _excludedBrokersForReplicaMove -> Handled by AbstractGoal
                                   ^
/home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/goals/BrokerSetAwareGoal.java:70: error: bad use of '>'
 * _requestedDestinationBrokerIds -> Handled by AbstractGoal
                                   ^
/home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/goals/BrokerSetAwareGoal.java:71: error: bad use of '>'
 * _onlyMoveImmigrantReplicas -> Handled by BrokerSetAwareGoa
                               ^
...

[mohitpali]

Done

[efeg]

@mohitpali Thanks for the PR!
I started the review and left some comments. I am midway through the review.

[efeg]

Added more comments -- will continue the review.

[CCisGG]

Hi @mohitpali , while you're updating the PR, I have some thoughts here to discuss/confirm with you.

Essentially here we need to solve 3 problems:

1. Which broker belongs to which broker set. [broker -> broker set]
2. Which topic belongs to which broker set. [topic -> broker set]
3. Which replica belongs to which broker. [replica -> broker]

For Prob.1, this PR provides a static file.
The major challenge then is what if the brokers are added or removed.

For Prob.2, this PR decides to move topic to it's original broker set, making an assumption of topic lives within a broker set at creation time.
The major issue of that solution is:

1. The assumption does not apply for most users other than AWS.
2. When there's no "original broker set", we still need to move all replicas to a single broker set.

For Prob.3, this PR allows the replicas goes to any eligible brokers in the target broker set. This looks fine for me.

Given the above observation, we may need to provide some pluggable component to solve Prob.1 and Prob.2. These are my current understanding. @mohitpali @efeg Please feel free to share your thoughts about it!

[mohitpali]

Hi @mohitpali , while you're updating the PR, I have some thoughts here to discuss/confirm with you.

Essentially here we need to solve 3 problems:

1. Which broker belongs to which broker set. [broker -> broker set]

2. Which topic belongs to which broker set.  [topic -> broker set]

3. Which replica belongs to which broker. [replica -> broker]

For Prob.1, this PR provides a static file. The major challenge then is what if the brokers are added or removed.

For Prob.2, this PR decides to move topic to it's original broker set, making an assumption of topic lives within a broker set at creation time. The major issue of that solution is:

1. The assumption does not apply for most users other than AWS.

2. When there's no "original broker set", we still need to move all replicas to a single broker set.

For Prob.3, this PR allows the replicas goes to any eligible brokers in the target broker set. This looks fine for me.

Given the above observation, we may need to provide some pluggable component to solve Prob.1 and Prob.2. These are my current understanding. @mohitpali @efeg Please feel free to share your thoughts about it!

I think this sounds correct. The outstanding issue is the topic to BrokerSet mapping policy - I will look into this.

[efeg]

Thanks for the updates -- left some comments.

[efeg]

Should we change _brokersByBrokerSet with something like _aliveBrokersByBrokerSet? That way we can drop the need for filtering here. As far as I can tell we never use brokers that are no alive for this goal.

[mohitpali]

I tried filtering this on the BrokerSetResolverHelper but the ExcludedBrokersForLeaderShipTest fails since it marks excluded brokers as dead and clusterModel.aliveBrokers() do not contain the excluded broker for leadership.

[CCisGG]

2 questions:

1. Could you point me to where it marks excluded brokers as dead?
2. Even if clusterModel.aliveBrokers() does not contain excluded broker, the test should succeed? E.g. if you have enough alive brokers, I don't see the reason why this goal can fail.

[efeg]

Made a full pass - left a few more comments.

[CCisGG]

Hi @mohitpali , I just realized another issue during my testing. It turns out that BrokerSetAwareGoal will naturally conflict with MinTopicLeadersPerBrokerGoal, as MinTopicLeadersPerBrokerGoal requires the topic exists on all brokers. As a result, optimization would fail whenever both goals are added as hard goals. I'm wondering if it's a good idea to exclude the topics defined in topics.with.min.leaders.per.broker when we do replica movements in BrokerSetAwareGoal. @efeg fyi.

[CCisGG]

Another observation is: if a broker dies, BrokerAwareGoal is still trying to rebalance for that broker. This is arguable.

When I implement my custom BrokerSetResolver, I would only color the alive brokers. With my implementation, today it's throwing "Failed to resolve BrokerSet for Broker Broker[id=XXX,rack=,state=DEAD" exception when the broker is dead.

I'm wondering if one should always assign all brokers with a color, dead or alive, in the custom BrokerSetResolver. If that's the case, maybe we can add something in the java doc in the BrokerSetResolver interface like "All brokers including dead ones should be assigned with a color".

(And sorry I accidentally closed the PR and reopened it... Hope that doesn't break anything)

[mohitpali]

Another observation is: if a broker dies, BrokerAwareGoal is still trying to rebalance for that broker. This is arguable.

When I implement my custom BrokerSetResolver, I would only color the alive brokers. With my implementation, today it's throwing "Failed to resolve BrokerSet for Broker Broker[id=XXX,rack=,state=DEAD" exception when the broker is dead.

I'm wondering if one should always assign all brokers with a color, dead or alive, in the custom BrokerSetResolver. If that's the case, maybe we can add something in the java doc in the BrokerSetResolver interface like "All brokers including dead ones should be assigned with a color".

(And sorry I accidentally closed the PR and reopened it... Hope that doesn't break anything)

I think it is because of below code -

  public String brokerSetId(Broker broker) throws BrokerSetResolutionException {
    String brokerSetId = _brokerSetIdByBroker.get(broker);

    if (brokerSetId != null) {
      return brokerSetId;
    }

    throw new BrokerSetResolutionException(String.format("Failed to resolve BrokerSet for Broker %s", broker));
  }

The reason the above code was introduced was since if a brokerSetId is ever found to be null, we would not be able to map the brokers for a brokerSetId. In your case I suspect since the Broker object changed during the goal run (state went from ALIVE to DEAD), the cached map did not have the mapping

private Map<Broker, String> _brokerSetIdByBroker;

I can modify it to instead map by BrokerId, and if the Broker object state has changed, it wouldn't throw this exception.

There could be other cases like race condition where during the goal run, a broker is added on the cluster but the brokerSet map does not have information on it. The BrokerSet resolution and ClusterModel's list of brokers may not match during this time. I would assume that a good brokerSet resolver is dynamic and even if a race condition fails optimization, next iteration of optimization will get fixed if the info has synched up.

[mohitpali]

Hi @mohitpali , I just realized another issue during my testing. It turns out that BrokerSetAwareGoal will naturally conflict with MinTopicLeadersPerBrokerGoal, as MinTopicLeadersPerBrokerGoal requires the topic exists on all brokers. As a result, optimization would fail whenever both goals are added as hard goals. I'm wondering if it's a good idea to exclude the topics defined in topics.with.min.leaders.per.broker when we do replica movements in BrokerSetAwareGoal. @efeg fyi.

I have excluded the topics with regex for MinTopicLeadersPerBrokerGoal

[CCisGG]

We should add this goal to cruisecontrol.properties, in the goals properties.

Make sure not adding this to default.goals and hard.goals.

[CCisGG]

Hi @mohitpali, I think there's a change needed in AnalyzerConfig to match the change in your latest commit. E.g. add the goal in the static DEFAULT_GOALS string. It should reflect/match the default values for "goals" property.

[CCisGG]

Hi @mohitpali, I know this is closed, but would like to ask your opinion on this:

Do you think it would be a good idea to "write back" the broker assignment to brokerSet.json file? That case no matter we have the original file or not, we can create/update the latest broker assignment info to that file, and then read it from anywhere we want.