[GitHub] [helix] pkuwm commented on a change in pull request #1416: add REST API for cluster topology

[GitBox <gi...@apache.org>]

pkuwm commented on a change in pull request #1416:
URL: https://github.com/apache/helix/pull/1416#discussion_r495427744



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
##########
@@ -125,6 +125,69 @@ public void testGetClusterTopology() {
   }
 
   @Test(dependsOnMethods = "testGetClusterTopology")
+  public void testGetClusterTopologyAndFaultZoneMap() throws IOException {
+    System.out.println("Start test :" + TestHelper.getTestMethodName());
+    String cluster = "TestCluster_1";
+    for (int i = 0; i < 5; i++) {
+      String instance = cluster + "localhost_129" + String.valueOf(18 + i);
+      HelixDataAccessor helixDataAccessor = new ZKHelixDataAccessor(cluster, _baseAccessor);
+      InstanceConfig instanceConfig =
+          helixDataAccessor.getProperty(helixDataAccessor.keyBuilder().instanceConfig(instance));
+      instanceConfig.setDomain("helixZoneId=zone0,instance=" + instance);
+      helixDataAccessor
+          .setProperty(helixDataAccessor.keyBuilder().instanceConfig(instance), instanceConfig);
+    }
+
+    for (int i = 0; i < 5; i++) {
+      String instance = cluster + "localhost_129" + String.valueOf(23 + i);
+      HelixDataAccessor helixDataAccessor = new ZKHelixDataAccessor(cluster, _baseAccessor);
+      InstanceConfig instanceConfig =
+          helixDataAccessor.getProperty(helixDataAccessor.keyBuilder().instanceConfig(instance));
+      instanceConfig.setDomain("helixZoneId=zone1,instance=" + instance);
+      helixDataAccessor
+          .setProperty(helixDataAccessor.keyBuilder().instanceConfig(instance), instanceConfig);
+    }
+
+    ClusterConfig configDelta = new ClusterConfig(cluster);
+    configDelta.getRecord().setSimpleField("TOPOLOGY", "/helixZoneId/instance");
+    updateClusterConfigFromRest(cluster, configDelta, Command.update);
+
+    //get cluster topology map
+    String topologyMapUrlBase = "clusters/TestCluster_1/topologymap/";
+    String topologyMapDef = get(topologyMapUrlBase, null, Response.Status.OK.getStatusCode(), true);
+    Map<String, Object> topologyMap =
+        OBJECT_MAPPER.readValue(topologyMapDef, new TypeReference<HashMap<String, Object>>() {
+        });
+    Assert.assertEquals(topologyMap.size(), 2);
+    Assert.assertTrue(topologyMap.get("/helixZoneId:zone0") instanceof List);
+    List<String> instances = (List<String>) topologyMap.get("/helixZoneId:zone0");
+    Assert.assertEquals(instances.size(), 5);
+
+    Assert.assertTrue(topologyMap.get("/helixZoneId:zone1") instanceof List);
+    instances = (List<String>) topologyMap.get("/helixZoneId:zone1");
+    Assert.assertEquals(instances.size(), 5);

Review comment:
       Do you think it'd be better to also verify the content of `instances`?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -442,6 +442,26 @@ public Response getClusterTopology(@PathParam("clusterId") String clusterId) thr
     return OK(objectMapper.writeValueAsString(clusterTopology));
   }
 
+  @ResponseMetered(name = HttpConstants.READ_REQUEST)
+  @Timed(name = HttpConstants.READ_REQUEST)
+  @GET
+  @Path("{clusterId}/topologymap")
+  public Response getClusterTopologyMap(@PathParam("clusterId") String clusterId) {
+    HelixAdmin admin = getHelixAdmin();
+    Map<String, List<String>> topologyMap = admin.getClusterTopology(clusterId).getTopologyMap();

Review comment:
       From the internal logic, if the cluster doesn't exist, an exception will be thrown - then it causes a server error 500, which is not appropriate, because it is a client error. Can we check if the cluster exists or not - if not, returns 404 NOT_FOUND and appropriate message
   ```
   {
    status_code: 404,
    message: cluster <clusterId> is not found
   }
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org