pkuwm commented on a change in pull request #1416:
URL: https://github.com/apache/helix/pull/1416#discussion_r495551579
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -442,6 +442,29 @@ 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) {
+ if (!doesClusterExist(clusterId)) {
+ return notFound(String.format("Cluster %s does not exist", clusterId));
+ }
+ HelixAdmin admin = getHelixAdmin();
+ Map<String, List<String>> topologyMap =
admin.getClusterTopology(clusterId).getTopologyMap();
+ return JSONRepresentation(topologyMap);
+ }
+
+ @ResponseMetered(name = HttpConstants.READ_REQUEST)
+ @Timed(name = HttpConstants.READ_REQUEST)
+ @GET
+ @Path("{clusterId}/faultzonemap")
Review comment:
From the java code logic, it seems these 2 REST APIs actually return a
topology which is represented by a map.
If so, I think the REST endpoint `{clusterId}/topology` is good and clear
enough? We don't need the `map` to specify the data format.
And for fault zone, how about adding a query parameter `domain_type` based
on the `/topology` endpoint: `/topology?domain_type=fault_zone`? (correct me
if I am not clear about it, as I just understand it by reading the code) Then
it looks cleaner and is easier to read.
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -442,6 +442,29 @@ 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) {
+ if (!doesClusterExist(clusterId)) {
+ return notFound(String.format("Cluster %s does not exist", clusterId));
+ }
+ HelixAdmin admin = getHelixAdmin();
+ Map<String, List<String>> topologyMap =
admin.getClusterTopology(clusterId).getTopologyMap();
+ return JSONRepresentation(topologyMap);
+ }
+
+ @ResponseMetered(name = HttpConstants.READ_REQUEST)
+ @Timed(name = HttpConstants.READ_REQUEST)
+ @GET
+ @Path("{clusterId}/faultzonemap")
+ public Response getClusterFaultZoneMap(@PathParam("clusterId") String
clusterId) {
+ HelixAdmin admin = getHelixAdmin();
+ Map<String, List<String>> topologyMap =
admin.getClusterTopology(clusterId).getTopologyMap();
Review comment:
Typo? `getTopologyMap` -> `getFaultZoneMap`.
And `getFaultZoneMap` throws an `IllegalArgumentException` when "The fault
zone in cluster config is not defined" - it seems to be client error. Would you
consider catching the exception and then returning a client error, instead of a
server error. I think it'd be better to differentiate a client error and a
server error.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
