controller static id and disable controller reblanace by default #202

yangy0000 · 2019-02-10T16:57:27Z

controller use static instead id instead of hostname
disable controller auto rebalance
fix bug in delete pipeline
fix typo in adminresource
update some opt name in manager admin resource to align with controller admin resource

docker-compose-example5.yml

xhl1988 · 2019-02-10T23:23:51Z

.../src/main/java/com/uber/stream/kafka/mirrormaker/controller/core/ManagerControllerHelix.java

+      if (znRecord == null) {
+        znRecord = new ZNRecord(resourcePath);
+      }
+      znRecord.setSimpleField(_instanceId, _hostname);


Can we set top level as something like "Controller id to hostname map"? We might need this map for worker as well. Also we might use propertyStore for other purpose in the future.

yupeng9

Thanks. I left some questions.

yupeng9 · 2019-02-11T04:36:03Z

uReplicator-Common/src/main/java/com/uber/stream/kafka/mirrormaker/common/utils/Constants.java

@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2015-2017 Uber Technologies, Inc. (streaming-data@uber.com)


Can we update this to 2019?

looks like license check will failed after I update 2017 to 2019, I will have a separate PR later to update all the license and license check.

You can update this https://github.com/uber/uReplicator/blob/master/pom.xml#L57 and then mvn license:format to update all of them. (in another PR)

uReplicator-Common/src/main/java/com/uber/stream/kafka/mirrormaker/common/utils/Constants.java

yupeng9 · 2019-02-11T04:42:58Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

@@ -294,14 +301,14 @@ private void validateInstanceToTopicPartitionsMap(
          }
        }
        if (mismatchTopicPartition.isEmpty()) {
-          LOGGER.info("Validate OK: InstanceName: {}, route: {}, #topics: {}, #partitions: {}, #workers: {}, worker: {}", instanceName, routeSet,
-              topicPartitions.size() - 1, partitionCount, instanceMap.get(instanceName).getWorkerSet().size(), instanceMap.get(instanceName).getWorkerSet());
+          LOGGER.info("Validate OK: InstanceName: {}, route: {}, #topics: {}, #partitions: {}, #workers: {}, worker: {}", hostname, routeSet,


Do you still want to keep InstanceName in the log message, or change it to hostname as well?

It might be useful to log both id and hostname so we can find the according host in deployment system even hostname is gone.

yupeng9 · 2019-02-11T04:46:05Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

-    for (String instanceName : instanceToTopicPartitionsMap.keySet()) {
-      Set<TopicPartition> topicPartitions = instanceToTopicPartitionsMap.get(instanceName);
+    for (String instanceId : instanceToTopicPartitionsMap.keySet()) {
+      String hostname = instanceIdAndNameMap.containsKey(instanceId) ? instanceIdAndNameMap.get(instanceId) : instanceId;


Not fully understand your intention here. Is this always a hostname, or just an instance name that be equal to hostname?

hostname should always be here after this change, but when we roll out the change, if we deploy manager before controller, hostname is not going to be found before controller change is rollout, so I put a place holder here to be safe.

yupeng9 · 2019-02-11T04:47:12Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

@@ -325,31 +332,31 @@ private void validateInstanceToTopicPartitionsMap(

            if (topicOnlyInManager.size() > 1 || (topicOnlyInManager.size() == 1 && !topicOnlyInManager.iterator().next().startsWith(SEPARATOR))) {
              validateWrongCount++;
-              LOGGER.error("Validate WRONG: InstanceName: {}, route: {}, topic only in manager: {}", instanceName, routeSet, topicOnlyInManager);
+              LOGGER.error("Validate WRONG: InstanceName: {}, route: {}, topic only in manager: {}", hostname, routeSet, topicOnlyInManager);


yupeng9 · 2019-02-11T04:47:18Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

            }

            if (!controllerTopics.isEmpty()) {
              validateWrongCount++;
-              LOGGER.error("Validate WRONG: InstanceName: {}, route: {}, topic only in controller: {}", instanceName, routeSet, controllerTopics);
+              LOGGER.error("Validate WRONG: InstanceName: {}, route: {}, topic only in controller: {}", hostname, routeSet, controllerTopics);


yupeng9 · 2019-02-11T05:01:18Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

-          instanceToReplace.add(instanceName);
+      // Check if any worker in route is down
+      boolean routeWorkerDown = false;
+      if (_enableRebalance || forceBalance) {


nit: it can reduce nested level (and would be easier to read), if you can write sth like

if (!(_enableRebalance || forceBalance)) return

there is autoscaling follow by autobalancing, so we could't return if auto balance is disabled .
I'm going to extract controller auto balancing and worker auto balancing logic separate function, this should help the readability

yupeng9 · 2019-02-11T05:03:58Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

+      PriorityQueue<InstanceTopicPartitionHolder> itphSet = _pipelineToInstanceMap.get(pipeline);
+      for (InstanceTopicPartitionHolder itph : itphSet) {
+        if (itph.getTotalNumPartitions() != 0) {
+          throw new UnsupportedOperationException("Delete non-empty pipeline is not allowed, serving number of partitions: "


Can you explain why disallow deleting non-empty pipeline? When can this happen, and what action to take when it happens?

this action might be triggered manually for pipeline cleanup. we might accidentally delete non-empty pipeline without this check, if so, topic in that pipeline will not been replicated.

got it. thanks

yupeng9 · 2019-02-11T05:06:34Z

...main/java/com/uber/stream/kafka/mirrormaker/manager/rest/resources/AdminRestletResource.java

@@ -88,12 +88,12 @@ public Representation post(Representation entity) {
      _helixMirrorMakerManager.disableAutoScaling();
      LOGGER.info("Disabled autoscaling!");
      responseJson.put("opt", "disable_autoscaling");
-      responseJson.put("auto_scaling", _helixMirrorMakerManager.isAutoBalancingEnabled());
+      responseJson.put("auto_scaling", _helixMirrorMakerManager.isAutoScalingEnabled());
    } else if ("enable_autoscaling".equalsIgnoreCase(opt)) {
      _helixMirrorMakerManager.enableAutoScaling();
      LOGGER.info("Enabled autobalancing!");
      responseJson.put("opt", "enable_autobalancing");


should this be enable_autoscaling?

yupeng9 · 2019-02-11T05:07:04Z

...main/java/com/uber/stream/kafka/mirrormaker/manager/rest/resources/AdminRestletResource.java

@@ -88,12 +88,12 @@ public Representation post(Representation entity) {
      _helixMirrorMakerManager.disableAutoScaling();
      LOGGER.info("Disabled autoscaling!");
      responseJson.put("opt", "disable_autoscaling");
-      responseJson.put("auto_scaling", _helixMirrorMakerManager.isAutoBalancingEnabled());
+      responseJson.put("auto_scaling", _helixMirrorMakerManager.isAutoScalingEnabled());
    } else if ("enable_autoscaling".equalsIgnoreCase(opt)) {
      _helixMirrorMakerManager.enableAutoScaling();
      LOGGER.info("Enabled autobalancing!");


update logger as well

xhl1988 · 2019-02-11T05:16:29Z

.../src/main/java/com/uber/stream/kafka/mirrormaker/controller/core/ManagerControllerHelix.java

+        znRecord = new ZNRecord(resourcePath);
+      }
+      znRecord.setSimpleField(_instanceId, _hostname);
+      propertyStore.set(resourcePath, znRecord, AccessOption.PERSISTENT);


Looks like it's setting the whole znRecord in each controller instead of incremental update, there might be race condition between different controllers.

Also is there any place which might use the id->hostname after controller is down? Currently we don't remove the mapping after controller is down.

xhl1988 · 2019-02-11T05:22:05Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

@@ -294,14 +301,14 @@ private void validateInstanceToTopicPartitionsMap(
          }
        }
        if (mismatchTopicPartition.isEmpty()) {
-          LOGGER.info("Validate OK: InstanceName: {}, route: {}, #topics: {}, #partitions: {}, #workers: {}, worker: {}", instanceName, routeSet,
-              topicPartitions.size() - 1, partitionCount, instanceMap.get(instanceName).getWorkerSet().size(), instanceMap.get(instanceName).getWorkerSet());
+          LOGGER.info("Validate OK: InstanceName: {}, route: {}, #topics: {}, #partitions: {}, #workers: {}, worker: {}", hostname, routeSet,


It might be useful to log both id and hostname so we can find the according host in deployment system even hostname is gone.

…et hostname in InstanceConfig is working

…s response

xhl1988 · 2019-02-12T05:01:41Z

...ger/src/main/java/com/uber/stream/kafka/mirrormaker/manager/core/ControllerHelixManager.java

+      // Check if any worker in route is down
+      boolean routeWorkerDown = false;
+      if (_enableRebalance || forceBalance) {
+        for (String instanceName : instanceToTopicPartitionsMap.keySet()) {


instanceId for consistency.

* controller static id and disable controller reblanace by default * update via comment * 1. update via comment 2. remove host name from property store since set hostname in InstanceConfig is working * update auto_balancing to autobalancing_status to align with get status response * fix ut failure * update parameter name for consistency

controller static id and disable controller reblanace by default

5cee4c3

yangy0000 requested a review from xhl1988 February 10, 2019 16:57

xhl1988 reviewed Feb 10, 2019

View reviewed changes

update via comment

bbc2c30

yupeng9 reviewed Feb 11, 2019

View reviewed changes

xhl1988 reviewed Feb 11, 2019

View reviewed changes

yangy0000 added 2 commits February 11, 2019 13:36

1. update via comment 2. remove host name from property store since s…

eb0f915

…et hostname in InstanceConfig is working

update auto_balancing to autobalancing_status to align with get statu…

255de01

…s response

yangy0000 requested review from yupeng9, xhl1988 and dongxiaoman February 11, 2019 21:44

fix ut failure

613af1d

yupeng9 approved these changes Feb 12, 2019

View reviewed changes

xhl1988 reviewed Feb 12, 2019

View reviewed changes

update parameter name for consistency

af2a5b8

xhl1988 approved these changes Feb 12, 2019

View reviewed changes

xhl1988 merged commit 1b0ce0f into master Feb 12, 2019

xhl1988 deleted the controller_instance_id branch February 12, 2019 15:48

xhl1988 mentioned this pull request Feb 12, 2019

Controller ID revisionRebased with latest master #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller static id and disable controller reblanace by default #202

controller static id and disable controller reblanace by default #202

yangy0000 commented Feb 10, 2019 •

edited

xhl1988 Feb 10, 2019

yupeng9 left a comment

yupeng9 Feb 11, 2019

yangy0000 Feb 11, 2019

yangy0000 Feb 11, 2019

xhl1988 Feb 11, 2019

yupeng9 Feb 11, 2019

xhl1988 Feb 11, 2019

yangy0000 Feb 11, 2019

yupeng9 Feb 11, 2019

yangy0000 Feb 11, 2019

yupeng9 Feb 11, 2019

yupeng9 Feb 11, 2019

yangy0000 Feb 11, 2019

yupeng9 Feb 11, 2019

yangy0000 Feb 11, 2019

yupeng9 Feb 11, 2019

yangy0000 Feb 11, 2019 •

edited

yupeng9 Feb 12, 2019

yupeng9 Feb 11, 2019

yupeng9 Feb 11, 2019

yangy0000 Feb 11, 2019

xhl1988 Feb 11, 2019

xhl1988 Feb 11, 2019

yangy0000 Feb 11, 2019

xhl1988 Feb 11, 2019

xhl1988 Feb 12, 2019

		@@ -0,0 +1,20 @@
		/*
		* Copyright (C) 2015-2017 Uber Technologies, Inc. (streaming-data@uber.com)

controller static id and disable controller reblanace by default #202

controller static id and disable controller reblanace by default #202

Conversation

yangy0000 commented Feb 10, 2019 • edited

Choose a reason for hiding this comment

yupeng9 left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangy0000 Feb 11, 2019 • edited

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

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

yangy0000 commented Feb 10, 2019 •

edited

yangy0000 Feb 11, 2019 •

edited