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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
