[GitHub] [helix] mgao0 commented on a diff in pull request #2127: Add HelixEventHandlingUtil and use that util for cloud event - Add API

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

mgao0 commented on code in PR #2127:
URL: https://github.com/apache/helix/pull/2127#discussion_r887310664


##########
helix-core/src/main/java/org/apache/helix/cloud/event/HelixEventHandlingUtil.java:
##########
@@ -0,0 +1,23 @@
+package org.apache.helix.cloud.event;
+
+import org.apache.helix.BaseDataAccessor;
+
+
+public class HelixEventHandlingUtil {
+
+  public static boolean enableInstanceForCloudEvent(String clusterName, String instanceName, boolean isEnable, long timeStamp,
+      BaseDataAccessor dataAccessor) {
+    return true;
+  }
+
+  public static boolean IsInstanceDisabledForCloudEvent(String clusterName, String instanceName,
+      BaseDataAccessor dataAccessor) {
+    return true;
+  }
+
+  public static Long getInstanceDisabledTimestamp(String clusterName, String instanceName,

Review Comment:
   Is this timestamp for cloud event disable or for the general disable?



##########
helix-core/src/main/java/org/apache/helix/cloud/event/HelixEventHandlingUtil.java:
##########
@@ -0,0 +1,23 @@
+package org.apache.helix.cloud.event;
+
+import org.apache.helix.BaseDataAccessor;
+
+
+public class HelixEventHandlingUtil {
+
+  public static boolean enableInstanceForCloudEvent(String clusterName, String instanceName, boolean isEnable, long timeStamp,
+      BaseDataAccessor dataAccessor) {
+    return true;
+  }
+
+  public static boolean IsInstanceDisabledForCloudEvent(String clusterName, String instanceName,

Review Comment:
   I understand why you name it like that - so it's more clear that this check is "for cloud event". But it is a bit counter intuitive because we return true for disabled, and return false for enabled. I'm thinking maybe we could still name it "enableInstance", "isInstanceEnabled", etc. But stress that these methods are for use cases under the cloud event context only in the method java doc, and should not be used for other purposes. What do you think?



##########
helix-core/src/main/java/org/apache/helix/cloud/event/HelixEventHandlingUtil.java:
##########
@@ -0,0 +1,23 @@
+package org.apache.helix.cloud.event;
+
+import org.apache.helix.BaseDataAccessor;
+
+
+public class HelixEventHandlingUtil {
+
+  public static boolean enableInstanceForCloudEvent(String clusterName, String instanceName, boolean isEnable, long timeStamp,

Review Comment:
   1. Open for discussion. I think our intention is to hide away these APIs from users, maybe we can restrict the access to package-only?
   2. Making a note on our "return false" or "throw exception" discussion when the action is failed
   3. Should we use the timestamp when the instance is actually enabled/disabled instead of when we initiate this move?



##########
helix-core/src/main/java/org/apache/helix/cloud/event/HelixEventHandlingUtil.java:
##########
@@ -0,0 +1,23 @@
+package org.apache.helix.cloud.event;
+
+import org.apache.helix.BaseDataAccessor;
+
+
+public class HelixEventHandlingUtil {

Review Comment:
   Is it class meant to be under `org.apache.helix.cloud.event.helix`?



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