Add a tool to map static cluster map information into Helix #523

pnarayanan · 2017-01-13T19:37:25Z

Add a tool to bootstrap and, from time to time, upgrade
cluster map information in Helix from the static clustermap files.
Adds tests to verify the tool's claims.
Adds certain functionalities in the test classes of the static
cluster map to help test the tool.

Part of #524

--
Notes:

Main changes to be reviewed are in HelixBootstrapUpgradeTool.java and HelixBootstrapUpgradeToolTest.java.
Tests cover all of the core functionalities.
We make use of a recently added method in Helix. This is available in Helix 0.6.7, which has not yet been published. For now, have commented out the applicable lines and marked TODOs that will be taken care of once 0.6.7 is out.

- Add a tool to bootstrap and, from time to time, upgrade cluster map information in Helix from the static clustermap files. - Adds tests to verify the tool's claims. - Adds certain functionalities in the test classes of the static cluster map to help test the tool.

vgkholla

Just quickly went through the scaffolding around the upgrade tool. Have to take a look at the tool and its test.

vgkholla · 2017-01-14T03:12:32Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

      for (int i = 0; i < datacenterCount; i++) {
        names.add(i, "DC" + i);
        dataNodes.add(i, getDataNodes(curBasePort, sslPort, getDisks()));
-        curBasePort += dataNodeCount;
+        curBasePort += 5000;


any reason to move to an absolute number?

have refactored this a bit. simply trying to partition things so that when new nodes are added at a later time (which is the new functionality added in these classes), the ports do not collide.

vgkholla · 2017-01-14T03:18:00Z

ambry-tools/src/main/java/com.github.ambry/tools/util/ToolUtils.java

@@ -81,4 +83,15 @@ public static Properties createConnectionPoolProperties() {
    props.put("connectionpool.connect.timeout.ms", "2000");
    return props;
  }
+
+  public static void ensureOrExit(List<OptionSpec> requiredArgs, OptionSet actualArgs, OptionParser parser)


vgkholla · 2017-01-14T03:19:37Z

gradle/dependency-versions.gradle

@@ -17,4 +17,5 @@ ext {
    commonsVersion = "1.5"
    bouncycastleVersion = "1.52"
    javaxVersion = "3.0.1"
+    helixVersion = "0.6.7-SNAPSHOT"


this can't be merged in this state right? Do we have to wait until the relevant version of helix is published?

Yeah - will figure this out (mentioned this in the description).

Will have to figure something out for the general case. For now, Helix is planning to cut a release later this week, so we are good.

vgkholla

Continuing to review ..

vgkholla · 2017-01-14T03:22:41Z

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

+ * The tool does the following:
+ * 1. bootstraps a static cluster map, adding nodes and partitions to Helix.
+ * 2. upgrades that involve new nodes and new partitions. To avoid over-complicating things, it assumes that the
+ *    existing partition assignment do not change during an upgrade. Newly added partitions can be distributed in any


does this also mean it assumes no new replicas are added for existing partitions?

Correct. We can make the change if required. At this time, I do not see a requirement as this never happens in the current usage of Ambry.

vgkholla · 2017-01-14T03:25:07Z

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

+  private final TreeMap<Long, Long> existingPartitions = new TreeMap<>();
+  private final TreeSet<Long> existingResources = new TreeSet<>();
+  private final String localDc;
+  private String clusterName;


haven't seen the code yet but just leaving a comment here in case i forget.

Can this be final?

yes, changed.

vgkholla · 2017-01-14T03:25:46Z

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

+  private final String localDc;
+  private String clusterName;
+
+  static final int MAX_PARTITIONS_PER_RESOURCE = 100;


what does this represent and why is it necessary?

This states the maximum number of Ambry partitions that will be grouped under a single Helix resource. See this section in the design.

vgkholla · 2017-01-14T03:28:04Z

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

+      listOpt.add(zkLayoutPathOpt);
+      ToolUtils.ensureOrExit(listOpt, options, parser);
+      bootstrapOrUpgrade(hardwareLayoutPath, partitionLayoutPath, zkLayoutPath, options.valueOf(localDcOpt));
+    } catch (Exception e) {


this catch isn't required. Since this is the main function, it will simply exit the program and print the stack trace of the exception (the stack trace is lost if you print it this way).

Ah, didn't know main() could throw - was simply following what other tools did.

vgkholla · 2017-01-14T03:37:55Z

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

+    try {
+      partitionLayoutString = Utils.readStringFromFile(partitionLayoutPath);
+    } catch (FileNotFoundException e) {
+      System.out.println("Partition layout path not found. Creating new file");


could we instead do

String partitionLayoutString = null; if (new File(partitionLayoutPath).exists()) { partitionLayoutString = Utils.readStringFromFile(partitionLayoutPath); }

?

I would vote against using exceptions for control flow.

In general, exceptions are implicitly used for control flow of the kind you have in mind, unless they are bail-out exceptions (which in this case it is not). Look at it this way - if lines 204 to 207 were encapsulated in a method, would you have had the same comment?
It so happens that File provides an exists() method, but if it hadn't, this exception will have to be used for control flow (JSON parser class for example, simply throws a JSONException if the contents are not valid JSON - and that might not be a bail-out case for the caller).

That said, your suggested change is much better, have made the change.

vgkholla · 2017-01-14T03:39:02Z

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

+    JSONArray all = (root).getJSONArray("zkHosts");
+    for (int i = 0; i < all.length(); i++) {
+      JSONObject entry = all.getJSONObject(i);
+      dataCenterToZkAddress.put(entry.getString("datacenter"),


could you define static constants for these strings?

vgkholla · 2017-01-14T03:41:56Z

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

+    for (int i = 0; i < all.length(); i++) {
+      JSONObject entry = all.getJSONObject(i);
+      dataCenterToZkAddress.put(entry.getString("datacenter"),
+          entry.getString("hostname") + ":" + entry.getString("port"));


if you are going to concatenate, instead of hostname and port, why not let the user provide an endpoint?

Just a more structured and extensible way of reading in fields in my opinion (say, if tomorrow the tool decides to make use of an API that takes host and port as arguments, or it makes a REST based connection).

nsivabalan

Went over HelixBootstrapUpgradeTool. Will review the rest in the 2nd pass once the feedback is addressed

nsivabalan · 2017-01-16T17:19:52Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

+      JSONObject jsonObject = jsonArray.getJSONObject(i);
+      currentPartitions.add((long) jsonObject.get("id"));
+    }
+    for (long i = 0; i < partitionCount; i++) {


minor: we can just start from currentPartitions.size() right ? Why do you want to start from 0?

Yes, thanks. Had started off with something more complex which ended up like this. Refactored.

nsivabalan · 2017-01-16T17:26:18Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

      jsonArray.put(getJsonPartition(i, partitionState, replicaCapacityInBytes, jsonReplicas));
    }
    return jsonArray;
  }

+  static JSONArray getUpdatedJsonPartitions(JSONArray jsonArray, long partitionCount,


ca you add javva docs for the methods added as part of this patch

nsivabalan · 2017-01-16T17:28:40Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

@@ -269,32 +271,52 @@ public static JSONObject getJsonPartition(long id, PartitionState partitionState
  }

  public static JSONArray getJsonPartitions(long partitionCount, PartitionState partitionState,
-      long replicaCapacityInBytes, int replicaCount, TestHardwareLayout testHardwareLayout) throws JSONException {
+      long replicaCapacityInBytes, int replicaCountPerDc, TestHardwareLayout testHardwareLayout) throws JSONException {


can you resolve the access specifiers for all the methods in this class using intellij. Most of can be made package private. Intellij will show them as yellow coded and will give suggestion as well

This file is not really core to this patch. Just added a method or so to help test the tool. If I go down the route of correcting these, this will become a huge patch.

Also, IntelliJ is basing these on what it knows about. You could argue that whoever authored a public method in TestUtils wanted it to be exposed outside of the package so that an outside component can make use of it (say if this tool was not part of the clustermap package) - even though currently no component outside needs it.

nsivabalan · 2017-01-16T17:29:03Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

      jsonArray.put(getJsonPartition(i, partitionState, replicaCapacityInBytes, jsonReplicas));
    }
    return jsonArray;
  }

+  static JSONArray getUpdatedJsonPartitions(JSONArray jsonArray, long partitionCount,


nsivabalan · 2017-01-16T17:30:13Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

+     * @param dataNodeCountPerDc number of datanodes to get from each datacenter.
+     * @return a list of datanodes.
+     */
+    public List<DataNode> getIndependentDataNodes(int dataNodeCountPerDc) {


package private

not sure if this was fixed?

sorry, fixed now.

nsivabalan · 2017-01-16T20:58:15Z

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

+      if (!admin.getClusters().contains(clusterName)) {
+        admin.addCluster(clusterName);
+        admin.addStateModelDef(clusterName, "LeaderStandby",
+            BuiltInStateModelDefinitions.LeaderStandby.getStateModelDefinition());


do you know what is the diff between this call and LeaderStandbySMD.build()

Effectively the same, though seems like LeaderStandbySMD.build() is the recommended way of defining this, so will use your suggestion (the other one eventually makes use of a deprecated method).

nsivabalan · 2017-01-16T21:00:57Z

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

+        InstanceConfig instanceConfig = dcAdmin.getInstanceConfig(clusterName, instanceName);
+        List<String> currentSealedPartitions = instanceConfig.getRecord().getListField("SEALED");
+        if (isSealed && !currentSealedPartitions.contains(partitionName)) {
+          List<String> newSealedPartitionsList = new ArrayList<>(currentSealedPartitions);


liens 363 to 366 is repeated in more than 1 place. do you think we can make it a private method

reorganized to avoid duplication - thanks.

nsivabalan · 2017-01-16T21:09:23Z

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

+   * tool groups together partitions under resources, with a limit to the number of partitions that will be grouped
+   * under a single resource.
+   */
+  private void updateHelix() {


may be "updateClusterMapInHelix"

nsivabalan · 2017-01-16T21:17:20Z

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

+        String[] instances = updateInstancesAndGetInstanceNames(dcAdmin, partitionName, replicaList, sealed);
+        resourceISBuilder.assignPreferenceList(partitionName, instances);
+      }
+      resourceISBuilder.setNumReplica(numReplicas);


each helix resource contains N partitions. So, what does num of replica means for a resource?

numReplicas is the number of replicas for every partitions in a resource (yes, all partitions of a resource has to have the same number of replicas).

nsivabalan · 2017-01-16T21:18:48Z

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

+  private String clusterName;
+
+  static final int MAX_PARTITIONS_PER_RESOURCE = 100;
+  static final String CAPACITY_STR = "capacityInBytes";


these could be private (93 to 100)

They are used by the test.

pnarayanan

Addressed feedback.

pnarayanan · 2017-01-17T23:04:15Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

@@ -269,32 +271,52 @@ public static JSONObject getJsonPartition(long id, PartitionState partitionState
  }

  public static JSONArray getJsonPartitions(long partitionCount, PartitionState partitionState,
-      long replicaCapacityInBytes, int replicaCount, TestHardwareLayout testHardwareLayout) throws JSONException {
+      long replicaCapacityInBytes, int replicaCountPerDc, TestHardwareLayout testHardwareLayout) throws JSONException {


This file is not really core to this patch. Just added a method or so to help test the tool. If I go down the route of correcting these, this will become a huge patch.

Also, IntelliJ is basing these on what it knows about. You could argue that whoever authored a public method in TestUtils wanted it to be exposed outside of the package so that an outside component can make use of it (say if this tool was not part of the clustermap package) - even though currently no component outside needs it.

pnarayanan · 2017-01-18T22:47:26Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

      jsonArray.put(getJsonPartition(i, partitionState, replicaCapacityInBytes, jsonReplicas));
    }
    return jsonArray;
  }

+  static JSONArray getUpdatedJsonPartitions(JSONArray jsonArray, long partitionCount,


pnarayanan · 2017-01-18T22:47:47Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

      jsonArray.put(getJsonPartition(i, partitionState, replicaCapacityInBytes, jsonReplicas));
    }
    return jsonArray;
  }

+  static JSONArray getUpdatedJsonPartitions(JSONArray jsonArray, long partitionCount,


pnarayanan · 2017-01-18T22:51:29Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

+      JSONObject jsonObject = jsonArray.getJSONObject(i);
+      currentPartitions.add((long) jsonObject.get("id"));
+    }
+    for (long i = 0; i < partitionCount; i++) {


Yes, thanks. Had started off with something more complex which ended up like this. Refactored.

pnarayanan · 2017-01-19T18:33:39Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

      for (int i = 0; i < datacenterCount; i++) {
        names.add(i, "DC" + i);
        dataNodes.add(i, getDataNodes(curBasePort, sslPort, getDisks()));
-        curBasePort += dataNodeCount;
+        curBasePort += 5000;


have refactored this a bit. simply trying to partition things so that when new nodes are added at a later time (which is the new functionality added in these classes), the ports do not collide.

pnarayanan · 2017-01-20T03:04:38Z

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

+        String[] instances = updateInstancesAndGetInstanceNames(dcAdmin, partitionName, replicaList, sealed);
+        resourceISBuilder.assignPreferenceList(partitionName, instances);
+      }
+      resourceISBuilder.setNumReplica(numReplicas);


numReplicas is the number of replicas for every partitions in a resource (yes, all partitions of a resource has to have the same number of replicas).

pnarayanan · 2017-01-20T03:05:05Z

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

+  /**
+   * Updates instances that hosts replicas of this partition with the replica information (including the mount points
+   * on which these replicas should reside, which will be purely an instance level information).
+   * @param dcAdmin the admin to the Zk server on which this operatin is to be done.


pnarayanan · 2017-01-20T03:09:15Z

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

+    for (int i = 0; i < replicaList.size(); i++) {
+      Replica replica = (Replica) replicaList.get(i);
+      DataNodeId node = replica.getDataNodeId();
+      String instanceName = node.getHostname() + "_" + node.getPort();


pnarayanan · 2017-01-20T03:12:24Z

ambry-tools/src/main/java/com.github.ambry/tools/util/ToolUtils.java

@@ -81,4 +83,15 @@ public static Properties createConnectionPoolProperties() {
    props.put("connectionpool.connect.timeout.ms", "2000");
    return props;
  }
+
+  public static void ensureOrExit(List<OptionSpec> requiredArgs, OptionSet actualArgs, OptionParser parser)


pnarayanan · 2017-01-20T03:13:23Z

gradle/dependency-versions.gradle

@@ -17,4 +17,5 @@ ext {
    commonsVersion = "1.5"
    bouncycastleVersion = "1.52"
    javaxVersion = "3.0.1"
+    helixVersion = "0.6.7-SNAPSHOT"


Will have to figure something out for the general case. For now, Helix is planning to cut a release later this week, so we are good.

pnarayanan · 2017-01-23T18:02:09Z

@vgkholla @nsivabalan please take a look when you get a chance. thanks.

vgkholla

LGTM. As discussed, it might be useful to have a verifier at the end of the tool itself to build confidence that the state in helix is consistent with the state in the static file.

vgkholla · 2017-01-24T01:43:05Z

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

+    parseZkJsonAndPopulateZkInfo(zkLayoutPath);
+
+    ClusterMapConfig clusterMapConfig = new ClusterMapConfig(new VerifiableProperties(new Properties()));
+    staticClusterMap =


ternary operator made this hard reading for me.

nsivabalan · 2017-01-24T22:27:16Z

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

+ *   ]
+ * }
+ *
+ * This tool should be run from an admin node that has access to the nodes in the hardware layout. The access is


do you think we should word it as "this tool should be run (in every datacenter) from an admin node..."

This needs to be run only once - does not have to be run from every datacenter.

oops. got you. I don't know why I made that comment.

nsivabalan · 2017-01-24T22:44:45Z

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

+  private final Map<String, String> dataCenterToZkAddress = new HashMap<>();
+  private final Map<String, HelixAdmin> adminForDc = new HashMap<>();
+  // A mapping from partitions to Helix resource, populated at the start with what is present in Helix.
+  private final TreeMap<Long, Long> existingPartitions = new TreeMap<>();


this is bit confusing. Can we name it partitionsToResourceMap. Also why do we need to append "existing" to this one and the next data structure. We don't have any other variable which stores upcoming partitions or resources. In that case, would like to keep it simple.

changed this to just a set of existing partitions, as the partition to resource mapping was unnecessary.
These are the partitions and resource that are already present in Helix when the tool starts up. That way I think "existing" is appropriate. These are not modified after initialization.

nsivabalan · 2017-01-24T22:49:51Z

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

+  private final String localDc;
+  private final String clusterName;
+
+  static final int MAX_PARTITIONS_PER_RESOURCE = 100;


Do you think this value should be made configurable ?

Made this an optional argument.

nsivabalan · 2017-01-24T22:50:45Z

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

+      // Add a cluster entry in every DC
+      if (!admin.getClusters().contains(clusterName)) {
+        admin.addCluster(clusterName);
+        admin.addStateModelDef(clusterName, "LeaderStandby", LeaderStandbySMD.build());


replace the string "LeaderStandby" with LeaderStandbySMD.name

nsivabalan · 2017-01-24T22:56:26Z

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

+        DataNodeId node = replicaId.getDataNodeId();
+        String instanceName = getInstanceName(node);
+        InstanceConfig instanceConfig = dcAdmin.getInstanceConfig(clusterName, instanceName);
+        List<String> currentSealedPartitions = instanceConfig.getRecord().getListField("SEALED");


nsivabalan · 2017-01-24T22:58:51Z

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

+    HelixBootstrapUpgradeTool clusterMapToHelixMapper =
+        new HelixBootstrapUpgradeTool(hardwareLayoutPath, partitionLayoutPath, zkLayoutPath, localDc);
+    clusterMapToHelixMapper.updateClusterMapInHelix();
+  }


after upgrade, can we close all the HelixAdmins that are currently open

nsivabalan

Done with prod code. Reviewing test code now

nsivabalan · 2017-01-24T23:07:11Z

ambry-clustermap/src/test/java/com.github.ambry.clustermap/TestUtils.java

+   * @param basePort the starting standard port number for nodes generated
+   * @param sslPort the starting SSL port number for nodes generated
+   * @param hardwareState a {@link HardwareLayout} value for each node
+   * @param disks a {@link JSONArray} of disks for each node


does every data node added has the same set of disks?

Yes, that is how TestUtils is written. I don't think that matters for the purposes of testing this tool.

nsivabalan · 2017-01-24T23:15:21Z

ambry-tools/src/test/java/com/github/ambry/clustermap/HelixBootstrapUpgradeToolTest.java

+    }
+
+    void shutdown() {
+      zkServer.shutdown();


don't we need to clean up the directories ?

they get deleted because a deleteOnExit() call is made on the root directory after creation.

nsivabalan · 2017-01-24T23:22:39Z

ambry-tools/src/test/java/com/github/ambry/clustermap/HelixBootstrapUpgradeToolTest.java

+   */
+  private void verifyDataNodeAndDiskEquivalencyInDc(Datacenter dc, String clusterName, HardwareLayout hardwareLayout,
+      PartitionLayout partitionLayout) {
+    ClusterMapManager staticClusterMap = new ClusterMapManager(partitionLayout);


its ironic that hardwareLayout is never used to check for data node and disk equivalency ;)

It gets used indirectly. the partitionLayout contains a reference to the associated hardwareLayout and that is what gets used. I have removed the unused variable from the signature.

yes I knew how it works :) just called it out

nsivabalan · 2017-01-24T23:27:17Z

gradle/dependency-versions.gradle

@@ -17,4 +17,5 @@ ext {
    commonsVersion = "1.5"
    bouncycastleVersion = "1.52"
    javaxVersion = "3.0.1"
+    helixVersion = "0.6.7"


is this a stable released version ?

this is the latest version as of this writing from the 0.6.x branch, which is the stable branch. This version itself has not yet been officially announced, but this has the changes we need.

nsivabalan

mostly minor comments only.

nsivabalan · 2017-01-24T23:31:21Z

ambry-tools/src/test/java/com/github/ambry/clustermap/HelixBootstrapUpgradeToolTest.java

+      IdealState resourceIS = admin.getResourceIdealState(clusterName, resourceName);
+      Set<String> resourcePartitions = resourceIS.getPartitionSet();
+      for (String resourcePartition : resourcePartitions) {
+        Assert.assertNull(


why assertNull ?

Validating that an Ambry partition is not seen under more than one resource.

nsivabalan · 2017-01-24T23:35:15Z

ambry-tools/src/test/java/com/github/ambry/clustermap/HelixBootstrapUpgradeToolTest.java

+    Map<String, Set<String>> allPartitionsToInstancesInHelix = new HashMap<>();
+    long numResources = 0;
+    for (String resourceName : admin.getResourcesInCluster(clusterName)) {
+      IdealState resourceIS = admin.getResourceIdealState(clusterName, resourceName);


do you verify stateModelName and numReplicas for each resource?

nsivabalan

Sorry couple of minor comments

pnarayanan · 2017-01-25T01:35:43Z

@nsivabalan @vgkholla added validation within the tool itself and tested it with a "real-life" cluster map; addressed other comments.

Looks like Helix 0.6.7 is not yet published, will follow up with the Helix team while you review the latest commit.

vgkholla

Minor comments. LGTM

vgkholla · 2017-01-25T07:25:39Z

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

  private static String getInstanceName(DataNodeId dataNode) {
    return dataNode.getHostname() + "_" + dataNode.getPort();
  }
+
+  void validateAndClose() {


vgkholla · 2017-01-25T07:33:04Z

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

+            "Replica information not consistent for instance " + instanceName + " disk " + disk.getMountPath()
+                + "\n in Helix: " + replicaList + "\n in static clustermap: " + replicasInClusterMap);
+      }
+


check for diskInfos.empty()?

pnarayanan · 2017-01-25T19:33:35Z

Incorporated comments, built and test in OS X and Linux.
Helix 0.6.7 has not yet been published (I am told it will be out in a week), so for now have commented out the applicable lines and marked TODOs so this can be merged.

pnarayanan · 2017-01-26T02:38:49Z

@nsivabalan could you merge this PR once you take a look at the latest changes?

xiahome · 2017-01-26T18:53:26Z

[update] Made a clean build and run for multiple times. Error was not reproduced.

I saw ./gradlew build intermittently fails. Error message is below.

:ambry-router:unitTest

com.github.ambry.router.DeleteManagerTest > testRouterClosedDuringOperation FAILED
    java.lang.AssertionError: Deletion should be unsuccessful. Exception is expected.
        at org.junit.Assert.fail(Assert.java:91)
        at com.github.ambry.router.DeleteManagerTest.assertFailureAndCheckErrorCode(DeleteManagerTest.java:430)
        at com.github.ambry.router.DeleteManagerTest.access$300(DeleteManagerTest.java:54)
        at com.github.ambry.router.DeleteManagerTest$4.testAndAssert(DeleteManagerTest.java:377)
        at com.github.ambry.router.RouterTestHelpers.testWithErrorCodes(RouterTestHelpers.java:79)
        at com.github.ambry.router.RouterTestHelpers.testWithErrorCodes(RouterTestHelpers.java:62)
        at com.github.ambry.router.DeleteManagerTest.testRouterClosedDuringOperation(DeleteManagerTest.java:371)

90 tests completed, 1 failed
:ambry-router:unitTest FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':ambry-router:unitTest'.
> There were failing tests. See the report at: file:///Users/mxia/ws/linkedin_git_ambry/ambry-router/build/reports/tests/index.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 1 mins 46.927 secs

pnarayanan · 2017-01-26T18:56:55Z

@xiahome this patch does not have any changes that should fail that test. Could you

run the test on a clean build
take a look at the test to see why it might be failing? I think you are more familiar with the tests in the DeleteManager.

nsivabalan · 2017-01-26T19:21:40Z

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

+        Collections.shuffle(Arrays.asList(instances));
+        resourceISBuilder.assignPreferenceList(partitionName, instances);
+      }
+      resourceISBuilder.setNumReplica(numReplicas);


Does resource have information about no of partitions it has ? Or just get the list of partitions and get the size of them ?

I think it is the latter.

nsivabalan · 2017-01-26T19:24:05Z

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

+            .describedAs("local_dc")
+            .ofType(String.class);
+
+    ArgumentAcceptingOptionSpec<String> maxPartitionsInOneResourceOpt = parser.accepts("maxPartitionsInOneResource",


Do we have any suggestion as to how to set this no? During different upgrades, the total partitions added might be different right. Do you think adding a line to documentation is required?

This does not have to be set. This is an override option, so the user would know.

nsivabalan · 2017-01-26T19:28:31Z

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

+   * @param hardwareLayout the {@link HardwareLayout} of the static clustermap.
+   * @param partitionLayout the {@link PartitionLayout} of the static clustermap.
+   */
+  private void verifyEquivalencyWithStatic(HardwareLayout hardwareLayout, PartitionLayout partitionLayout) {


may be rename to "verifyEquivalencyWithStaticClusterMap" or "verifyEquivalencyWithStaticCS"

nsivabalan · 2017-01-26T19:32:38Z

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

@@ -199,15 +210,18 @@ static void bootstrapOrUpgrade(String hardwareLayoutPath, String partitionLayout
   * @throws JSONException if there is an error parsing the JSON content in any of the files.
   */
  private HelixBootstrapUpgradeTool(String hardwareLayoutPath, String partitionLayoutPath, String zkLayoutPath,
-      String localDc) throws IOException, JSONException {
+      String localDc, int maxPartitionsInOneResource) throws IOException, JSONException {


java doc for this param

nsivabalan · 2017-01-26T19:35:38Z

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

+   * @param clusterName the cluster to be verified.
+   * @param partitionLayout the {@link PartitionLayout} of the static clustermap.
+   */
+  private void verifyDataNodeAndDiskEquivalencyInDc(Datacenter dc, String clusterName,


I assume these verify methods are copied from the test class and hence I am not reviewing once again. If there are many changes, let me know

yeah, they are the same.

nsivabalan · 2017-01-26T19:36:53Z

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

+              + replicaHostsInHelix);
+    }
+    ensureOrThrow(allPartitionsToInstancesInHelix.isEmpty(),
+        "More partitions in Helix than in clustermap, additional" + " partitions: "


remove unwanted "+" (string concatenation)

nsivabalan · 2017-01-26T19:38:10Z

@pnarayanan : very few minor comments. Once you are done, will merge it

pnarayanan · 2017-01-26T19:52:57Z

@nsivabalan incorporated comments. built and formatted.

nsivabalan · 2017-01-26T22:48:35Z

Looks good. Merging. Can you confirm that you built using java 1.7 too?

pnarayanan · 2017-01-26T22:50:16Z

Yes, I always build with 1.7. Builds in both Linux and OS X.

pnarayanan added the enhancement label Jan 13, 2017

pnarayanan added this to the Dynamic Cluster Management milestone Jan 13, 2017

pnarayanan requested review from vgkholla and nsivabalan January 13, 2017 19:37

vgkholla reviewed Jan 14, 2017

View reviewed changes

nsivabalan reviewed Jan 17, 2017

View reviewed changes

Incorporate review comments

5958122

pnarayanan commented Jan 20, 2017

View reviewed changes

Using Helix 0.6.7

ffb4f8e

Shuffle preference list of instances before assigning to resource IS

1fdb745

vgkholla reviewed Jan 24, 2017

View reviewed changes

nsivabalan reviewed Jan 24, 2017

View reviewed changes

Added validation in the tool itself; incorporated other comments

8db1416

vgkholla approved these changes Jan 25, 2017

View reviewed changes

Incorporate comments, switch back to Helix 0.6.6 for now

c198164

nsivabalan reviewed Jan 26, 2017

View reviewed changes

Incorporate Siva's comments

d81113f

nsivabalan merged commit a98d757 into linkedin:master Jan 26, 2017

pnarayanan deleted the helixTool branch January 28, 2017 20:00

Add a tool to map static cluster map information into Helix #523

Add a tool to map static cluster map information into Helix #523

Conversation

pnarayanan commented Jan 13, 2017 • edited

vgkholla 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

vgkholla 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

nsivabalan 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

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

pnarayanan 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

pnarayanan commented Jan 23, 2017

vgkholla 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

nsivabalan Jan 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnarayanan commented Jan 13, 2017 •

edited

nsivabalan Jan 24, 2017 •

edited