Add simple logic locality-awareness reassignments #116

kabochya · 2019-03-28T03:15:50Z

This is the first part for locality-aware reassignments using rackId/availability zone info. Currently it will not failover to other localities nor will it balance replicas throughout the localities. These issues will be issued afterwards.

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaCluster.java

drkafka/src/test/java/com/pinterest/doctorkafka/KafkaClusterTest.java

ambud · 2019-04-02T02:50:38Z

@kabochya could you please squash the commits?

yuyang08 · 2019-04-02T06:52:18Z

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaCluster.java

+      }
+    }
+
+    // Second pass to fill in replacements without target brokers in locality


is this still work-in-progress?

This is a ready PR, it implements trivial logic without handling cases such as failover to other localities if no instances are available in the current locality, and spreading out the reassignments to different localities if there are multiple replicas of the same topic partition on the dead broker that is in the same locality. These issues will be addressed in subsequent PRs.

yuyang08 · 2019-04-03T05:49:42Z

drkafka/src/test/java/com/pinterest/doctorkafka/KafkaClusterTest.java

@@ -12,34 +12,37 @@
 import org.apache.kafka.common.Node;
 import org.apache.kafka.common.PartitionInfo;
 import org.apache.kafka.common.TopicPartition;
+import org.junit.jupiter.api.AfterAll;


remove this unused import ?

yuyang08 · 2019-04-03T05:49:59Z

drkafka/src/test/java/com/pinterest/doctorkafka/KafkaClusterTest.java

 import org.junit.jupiter.api.Test;

+import java.sql.Time;


remove this unused import ?

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java

yuyang08 · 2019-04-03T06:11:23Z

drkafka/src/test/java/com/pinterest/doctorkafka/KafkaClusterTest.java

-    ReplicaStatsManager.bytesInStats = oldBytesInStats;
-    ReplicaStatsManager.bytesOutStats = oldBytesOutStats;
+  @Test
+  void testLocalityAwareReassignments() throws Exception {


could we add another test to test the scenario that there is not enough capacity in the rack for assigning the partition?

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaCluster.java

yuyang08 · 2019-04-03T06:18:31Z

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaCluster.java

    }
    // push the brokers back to brokerQueue to keep invariant true
    brokerQueue.addAll(unusableBrokers);
    return success ? result : null;
  }

+  /**
+   * Similar to getAlternativeBrokers, but locality aware


a general question: when broker.rack is not set on the kafka cluster side, kafkastats will have null for rackId. In that case, if the user set locality_awareness.enabled to true in the setting, what will be the expected behavior? can we add explanation about this in the comment?

drkafka/src/main/java/com/pinterest/doctorkafka/config/DoctorKafkaClusterConfig.java

kabochya · 2019-04-15T20:10:49Z

@yuyang08
Added notifications for failure on locality-aware reassignments, however the partital reassignment strategy (still assigning oosReplicas without caring if some other oosReplicas fail to get reassigned). This behaviour is inconsistent with the original one, which is drop the whole reassignment if even one oosReplica failed to get reassigned. Should we unify the behavior of these two methods? I have documented the differences in the comments of the generateReassignmentPlanForDeadBrokers method.

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaCluster.java

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java

yuyang08 · 2019-04-26T09:34:03Z

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java

      if (replacedNodes == null) {
+        if (isLocalityAware) {
+          continue;


with this, will we exit the loop with success == true ?

in that case, will we get an email alert if doctorkafka failes to find a locality aware assignment?

@yuyang08 Yes, we will get a different email alert if the assignment partially failed and these failure will be captured in the hashmap reassignmentToLocalityFailures. L672-680 will then check if there are partial failures during locality aware reassignments and alert by email.

Added a guard to set success to false if no locality-aware reassignments were made.

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java

Unit test for testing SINGLE oosReplica

yuyang08

thanks for making the fix!!

kabochya · 2019-04-30T17:49:48Z

Removed partial reassignments

ambud suggested changes Mar 31, 2019

View reviewed changes

kabochya requested a review from yuyang08 April 1, 2019 18:56

yuyang08 reviewed Apr 2, 2019

View reviewed changes

yuyang08 reviewed Apr 3, 2019

View reviewed changes

kabochya force-pushed the feature/locality-aware-reassignment branch 3 times, most recently from 10a8edf to f7a67ce Compare April 15, 2019 18:42

kabochya force-pushed the feature/locality-aware-reassignment branch from f7a67ce to 2aa0ff4 Compare April 15, 2019 20:13

kabochya commented Apr 16, 2019

View reviewed changes

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java Outdated Show resolved Hide resolved

yuyang08 reviewed Apr 23, 2019

View reviewed changes

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaCluster.java Outdated Show resolved Hide resolved

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java Outdated Show resolved Hide resolved

kabochya force-pushed the feature/locality-aware-reassignment branch from 2aa0ff4 to 308725c Compare April 24, 2019 20:38

yuyang08 reviewed Apr 26, 2019

View reviewed changes

kabochya force-pushed the feature/locality-aware-reassignment branch from 308725c to 941b142 Compare April 26, 2019 17:02

yuyang08 reviewed Apr 30, 2019

View reviewed changes

drkafka/src/main/java/com/pinterest/doctorkafka/KafkaClusterManager.java Outdated Show resolved Hide resolved

Add locality-awareness reassignments.

34a4665

Unit test for testing SINGLE oosReplica

kabochya force-pushed the feature/locality-aware-reassignment branch from 941b142 to 34a4665 Compare April 30, 2019 17:28

yuyang08 approved these changes Apr 30, 2019

View reviewed changes

kabochya merged commit e1e7139 into pinterest:master Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple logic locality-awareness reassignments #116

Add simple logic locality-awareness reassignments #116

kabochya commented Mar 28, 2019

ambud commented Apr 2, 2019

yuyang08 Apr 2, 2019

kabochya Apr 2, 2019 •

edited

Loading

yuyang08 Apr 3, 2019

yuyang08 Apr 3, 2019

yuyang08 Apr 3, 2019

yuyang08 Apr 3, 2019

kabochya commented Apr 15, 2019

yuyang08 Apr 26, 2019

kabochya Apr 26, 2019

kabochya Apr 26, 2019

yuyang08 left a comment

kabochya commented Apr 30, 2019 •

edited

Loading

Add simple logic locality-awareness reassignments #116

Add simple logic locality-awareness reassignments #116

Conversation

kabochya commented Mar 28, 2019

ambud commented Apr 2, 2019

Choose a reason for hiding this comment

kabochya Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kabochya commented Apr 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyang08 left a comment

Choose a reason for hiding this comment

kabochya commented Apr 30, 2019 • edited Loading

kabochya Apr 2, 2019 •

edited

Loading

kabochya commented Apr 30, 2019 •

edited

Loading