[GitHub] [helix] junkaixue commented on a change in pull request #1995: Implement DefaultCloudEventCallbackImpl

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

junkaixue commented on a change in pull request #1995:
URL: https://github.com/apache/helix/pull/1995#discussion_r834510291



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java
##########
@@ -43,27 +48,40 @@ public void disableInstance(HelixManager manager, Object eventInfo) {
    * @param eventInfo Detailed information about the event
    */
   public void enableInstance(HelixManager manager, Object eventInfo) {
-    // To be implemented
-    throw new NotImplementedException();
+    manager.getClusterManagmentTool()
+        .enableInstance(manager.getClusterName(), manager.getInstanceName(), true);
   }
 
   /**
-   *
+   * Put cluster into maintenance mode if the cluster is not currently in maintenance mode
    * @param manager The helix manager associated with the listener
    * @param eventInfo Detailed information about the event
    */
   public void enterMaintenanceMode(HelixManager manager, Object eventInfo) {
-    // To be implemented
-    throw new NotImplementedException();
+    if (!manager.getClusterManagmentTool().isInMaintenanceMode(manager.getClusterName())) {
+      manager.getClusterManagmentTool()
+          .manuallyEnableMaintenanceMode(manager.getClusterName(), true, REASON, null);
+    }
   }
 
   /**
-   *
+   * Exit maintenance mode for the cluster, if there is no more live instances disabled for cloud event
    * @param manager The helix manager associated with the listener
    * @param eventInfo Detailed information about the event
    */
   public void exitMaintenanceMode(HelixManager manager, Object eventInfo) {

Review comment:
       I am thinking about this could still be a concern for race condition. Say two instances are ready to enable, both of them check instantly and find another instance is disabled. So both of them just disabled without exiting maintenance mode.
   
   I think we may need to introduce some centralized locking to avoid the race condition.

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java
##########
@@ -19,22 +19,27 @@
  * under the License.
  */
 
+import java.util.List;
+
 import org.apache.helix.HelixManager;
-import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+import org.apache.helix.constants.InstanceConstants;
+import org.apache.helix.util.InstanceValidationUtil;
 
 /**
  * A default callback implementation class to be used in {@link HelixCloudEventListener}
  */
 public class DefaultCloudEventCallbackImpl {
+  public static final String REASON = "CloudEvent";

Review comment:
       Let's have a detailed reason and put to maintenance mode / instance disable reason?




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