Added HelixPopulateTool and tests. #1176

zzmao · 2019-05-30T20:44:32Z

Add HelixPopulateTool to create or update a helix cluster zk info.

codecov-io · 2019-05-30T20:59:06Z

Codecov Report

Merging #1176 into master will decrease coverage by 0.19%.
The diff coverage is 1.16%.

@@             Coverage Diff             @@
##             master    #1176     +/-   ##
===========================================
- Coverage     69.86%   69.67%   -0.2%     
+ Complexity     5374     5371      -3     
===========================================
  Files           428      429      +1     
  Lines         32863    32949     +86     
  Branches       4147     4160     +13     
===========================================
- Hits          22960    22957      -3     
- Misses         8772     8856     +84     
- Partials       1131     1136      +5

Impacted Files	Coverage Δ	Complexity Δ
....github.ambry/clustermap/HelixVcrPopulateTool.java	`0% <0%> (ø)`	`0 <0> (?)`
...ithub.ambry.cloud/azure/AzureCloudDestination.java	`81.67% <100%> (+0.09%)`	`26 <0> (ø)`	⬇️
.../main/java/com.github.ambry.router/EncryptJob.java	`92.1% <0%> (-5.27%)`	`4% <0%> (-1%)`
...java/com.github.ambry.store/CompactionManager.java	`87.33% <0%> (-2.67%)`	`19% <0%> (ø)`
.../main/java/com.github.ambry.router/PutManager.java	`87.42% <0%> (-0.63%)`	`29% <0%> (ø)`
...src/main/java/com.github.ambry.commons/BlobId.java	`93.18% <0%> (-0.36%)`	`71% <0%> (-1%)`
...java/com.github.ambry.network/SSLTransmission.java	`69.42% <0%> (-0.32%)`	`68% <0%> (-1%)`
...in/java/com.github.ambry.store/BlobStoreStats.java	`71.54% <0%> (-0.21%)`	`103% <0%> (ø)`
...va/com.github.ambry.replication/ReplicaThread.java	`74.52% <0%> (-0.19%)`	`65% <0%> (-1%)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6373104...3f9ada3. Read the comment docs.

jsjtzyy · 2019-05-30T23:10:35Z

ambry-tools/src/integration-test/java/com.github.ambry/clustermap/HelixVcrPopulateToolTest.java

@@ -0,0 +1,126 @@
+/*
+ * Copyright 2017 LinkedIn Corp. All rights reserved.


2019, please

jsjtzyy · 2019-05-30T23:11:10Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

@@ -0,0 +1,183 @@
+/*
+ * Copyright 2017 LinkedIn Corp. All rights reserved.


jsjtzyy · 2019-05-31T16:52:08Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+  private static String SEPARATOR = "/";
+  static List<String> ignoreResourceKeyWords = Arrays.asList("aggregation", "trigger", "stats");
+
+  public static void main(String args[]) {


use java style: String[] args

jsjtzyy · 2019-05-31T16:58:49Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+    OptionSpec createClusterOpt = parser.accepts("createCluster",
+        "Create resources in dest by copying from src to dest. --createCluster --dest destZkEndpoint/destClusterName");
+    OptionSpec updateClusterOpt = parser.accepts("updateCluster",
+        "Update resources in dest by copying from src to dest. --updateCluster"


Still don't quite understand Create resources in dest by copying from src to dest in createCluster. I feel like maybe we can change the description a little bit. Let's discuss offline.

no copy required. Just create. Update.

jsjtzyy · 2019-05-31T17:34:42Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+    if (options.has(updateClusterOpt)) {
+      System.out.println("Updating cluster: " + destClusterName);
+      String srcZkString = options.valueOf(srcOpt).split(SEPARATOR)[0];
+      String srcClusterName = options.valueOf(srcOpt).split(SEPARATOR)[1];


move System.out.println("Updating cluster: " + destClusterName); here and add more info like " from existing cluster: " + srcClusterName .

jsjtzyy · 2019-05-31T18:03:31Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+    ClusterConfig clusterConfig = configAccessor.getClusterConfig(destClusterName);
+    clusterConfig.setPersistBestPossibleAssignment(true);
+    configAccessor.setClusterConfig(destClusterName, clusterConfig);
+    System.out.println("Cluster " + destClusterName + " create done.");


minor: how about this:
System.out.println("Cluster " + destClusterName + " is successfully created!");

jsjtzyy · 2019-05-31T18:04:05Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+   * @param destClusterName the dest cluster's name
+   */
+  static void updateResourceAndPartition(String srcZkString, String srcClusterName, String destZkString,
+      String destClusterName, boolean dryRun) {


add java doc for dryRun

jsjtzyy · 2019-05-31T18:06:19Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+  }
+
+  /**
+   * Update dest cluster information based on src cluster. Dest cluster resource will be recreated if src cluster has any change.


minor question: is recreated automatically? In other words, any changes in src cluster will trigger changes in dest cluster automatically?

Never mind, I understand what you mean here by reading following code. However, I feel like it could be better to change the description as:
Dest cluster resource will be recreated if it mismatches that in src cluster.

jsjtzyy · 2019-05-31T18:10:35Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+        }
+      }
+    }
+    System.out.println("Cluster " + destClusterName + " update done.");


minor: how about System.out.println("Cluster " + destClusterName + " is successfully updated!");

jsjtzyy · 2019-05-31T18:37:23Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+        Set<String> srcPartitions = srcAdmin.getResourceIdealState(srcClusterName, resource).getPartitionSet();
+        Set<String> destPartitions = destAdmin.getResourceIdealState(destClusterName, resource).getPartitionSet();
+        for (String partition : srcPartitions) {
+          if (!destPartitions.contains(partition)) {


what if destPartitions has partition that is not in srcPartitions?

zzmao · 2019-06-03T19:34:37Z

@jsjtzyy Addressed your comments and enhanced a test.

lightningrob · 2019-06-05T00:16:47Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+    String destZkString = options.valueOf(destOpt).split(SEPARATOR)[0];
+    String destClusterName = options.valueOf(destOpt).split(SEPARATOR)[1];
+    if (!destClusterName.contains("VCR")) {
+      System.out.println("dest should be a VCR cluster.(VCR string should be included)");


System.err.println.

lightningrob · 2019-06-05T00:17:56Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+    destZkClient.setZkSerializer(new ZNRecordSerializer());
+    HelixAdmin destAdmin = new ZKHelixAdmin(destZkClient);
+    if (destAdmin.getClusters().contains(destClusterName)) {
+      System.out.println("Failed to create cluster becuase " + destClusterName + " already exist.");


lightningrob · 2019-06-05T00:19:02Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+  public static void main(String[] args) {
+    OptionParser parser = new OptionParser();
+    OptionSpec createClusterOpt = parser.accepts("createCluster",
+        "Create resources in dest cluster. --createCluster --dest destZkEndpoint/destClusterName");


Does it actually create resources or just the cluster?

just cluster.

Will add in comment.

Issues addressed in #1179

lightningrob · 2019-06-05T00:29:13Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+          System.out.println("DryRun: Drop Resource " + resource);
+        } else {
+          // This resource need to be recreate.
+          destAdmin.dropResource(destClusterName, resource);


It looks like this should only be done if the resource exists in dest and is different from src. You might need another boolean variable like dropChangedResource to condition this action.

lightningrob · 2019-06-05T00:30:30Z

ambry-tools/src/main/java/com.github.ambry/clustermap/HelixVcrPopulateTool.java

+          System.out.println("DryRun: Add Resource " + resource + " with partition " + srcPartitions);
+        } else {
+          destAdmin.addResource(destClusterName, resource, idealState);
+          destAdmin.rebalance(destClusterName, resource, 3, "", "");


Not obvious what constant 3 refers to. Variable or comment would be helpful.

lightningrob

Please address comments in follow up PR.

Fix some issue in #1176 Add controlResource and maintainCluster support.

zzmao requested a review from jsjtzyy May 30, 2019 20:44

zzmao force-pushed the helixTool branch from de44636 to 3f9ada3 Compare May 31, 2019 00:36

Added HelixPopulateTool and tests.

37024b8

jsjtzyy reviewed May 31, 2019

View reviewed changes

address yingyi comment

6507055

zzmao force-pushed the helixTool branch from 3f9ada3 to 6507055 Compare June 3, 2019 19:33

jsjtzyy approved these changes Jun 3, 2019

View reviewed changes

jsjtzyy merged commit 3ee2f2a into linkedin:master Jun 3, 2019

lightningrob reviewed Jun 5, 2019

View reviewed changes

zzmao mentioned this pull request Jun 5, 2019

Add resource control enable/disable #1179

Merged

lightningrob reviewed Jun 5, 2019

View reviewed changes

jsjtzyy pushed a commit that referenced this pull request Jun 5, 2019

Add resource control enable/disable (#1179)

85fb467

Fix some issue in #1176 Add controlResource and maintainCluster support.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added HelixPopulateTool and tests. #1176

Added HelixPopulateTool and tests. #1176

zzmao commented May 30, 2019

codecov-io commented May 30, 2019 •

edited

Loading

jsjtzyy May 30, 2019

jsjtzyy May 30, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

zzmao Jun 3, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

jsjtzyy May 31, 2019

zzmao commented Jun 3, 2019 •

edited

Loading

lightningrob Jun 5, 2019

lightningrob Jun 5, 2019

lightningrob Jun 5, 2019

zzmao Jun 5, 2019

zzmao Jun 5, 2019

zzmao Jun 5, 2019

lightningrob Jun 5, 2019

lightningrob Jun 5, 2019

lightningrob left a comment

		@@ -0,0 +1,126 @@
		/*
		* Copyright 2017 LinkedIn Corp. All rights reserved.

		@@ -0,0 +1,183 @@
		/*
		* Copyright 2017 LinkedIn Corp. All rights reserved.

Added HelixPopulateTool and tests. #1176

Added HelixPopulateTool and tests. #1176

Conversation

zzmao commented May 30, 2019

codecov-io commented May 30, 2019 • edited Loading

Codecov Report

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

zzmao commented Jun 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightningrob left a comment

Choose a reason for hiding this comment

codecov-io commented May 30, 2019 •

edited

Loading

zzmao commented Jun 3, 2019 •

edited

Loading