[GitHub] [helix] huizhilu commented on a change in pull request #1807: Add REST APIs for management mode

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

huizhilu commented on a change in pull request #1807:
URL: https://github.com/apache/helix/pull/1807#discussion_r659510448



##########
File path: helix-core/src/main/java/org/apache/helix/HelixAdmin.java
##########
@@ -381,6 +381,17 @@ void manuallyEnableMaintenanceMode(String clusterName, boolean enabled, String r
    */
   void setClusterManagementMode(ClusterManagementModeRequest request);
 
+  /**
+   * Gets cluster management status {@link ClusterManagementMode}: what mode the cluster is and
+   * whether the cluster has fully reached to that mode.
+   *
+   * @param clusterName cluster name
+   * @return {@link ClusterManagementMode}
+   */
+  default ClusterManagementMode getClusterManagementMode(String clusterName) {

Review comment:
       It's for backwards compatibility. Because some users implements the `HelixAdmin` interface for MockHelixAdmin in their code, it would break the build when bumping up the new helix lib.

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -297,6 +305,113 @@ public Response updateCluster(@PathParam("clusterId") String clusterId,
     return OK();
   }
 
+  @ResponseMetered(name = HttpConstants.READ_REQUEST)
+  @Timed(name = HttpConstants.READ_REQUEST)
+  @GET
+  @Path("{clusterId}/management-mode")
+  public Response getClusterManagementMode(@PathParam("clusterId") String clusterId,
+      @QueryParam("showDetails") boolean showDetails) {
+    ClusterManagementMode mode = getHelixAdmin().getClusterManagementMode(clusterId);
+    if (mode == null) {
+      return notFound("Cluster " + clusterId + " is not in management mode");

Review comment:
       It's aligned with the existing response for maintenance mode: `return notFound(String.format("Cluster %s is not in maintenance mode!", clusterId));`
   

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -521,6 +522,15 @@ public void setClusterManagementMode(ClusterManagementModeRequest request) {
     }
   }
 
+  @Override
+  public ClusterManagementMode getClusterManagementMode(String clusterName) {
+    HelixDataAccessor accessor =

Review comment:
       That's in helix-rest, not in this ZKHelixAdmin implementation. In helix rest, we can use getHelixDataAccessor()




-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

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