You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/06/15 17:14:46 UTC
[GitHub] [helix] qqu0127 commented on a diff in pull request #2149: implement util for cloud event
qqu0127 commented on code in PR #2149:
URL: https://github.com/apache/helix/pull/2149#discussion_r898223212
##########
helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java:
##########
@@ -19,51 +19,109 @@
* under the License.
*/
+import java.util.List;
+import java.util.Map;
+
+import org.apache.helix.HelixDataAccessor;
import org.apache.helix.HelixManager;
-import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+import org.apache.helix.constants.InstanceConstants;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.util.ConfigStringUtil;
+import org.apache.helix.util.InstanceValidationUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
/**
* A default callback implementation class to be used in {@link HelixCloudEventListener}
*/
public class DefaultCloudEventCallbackImpl {
+ private static final Logger LOG = LoggerFactory.getLogger(DefaultCloudEventCallbackImpl.class);
+ private final String _instanceReason = "Cloud event in DefaultCloudEventCallback at %s";
+ private final String _emmReason = "Cloud event EMM in DefaultCloudEventCallback by %s at %s";
/**
- * Disable the instance
+ * Disable the instance and track the cloud event in map field disabledInstancesWithInfo in
+ * cluster config. Will not re-disable the instance if the instance is already disabled for
+ * other reason. (So we will not overwrite the disabled reason and enable this instance when
+ * on-unpause)
* @param manager The helix manager associated with the listener
* @param eventInfo Detailed information about the event
*/
public void disableInstance(HelixManager manager, Object eventInfo) {
- // To be implemented
- throw new NotImplementedException();
+ String message = String.format(_instanceReason, System.currentTimeMillis());
+ LOG.info("DefaultCloudEventCallbackImpl disabled Instance {}", manager.getInstanceName());
+ if (InstanceValidationUtil
+ .isEnabled(manager.getHelixDataAccessor(), manager.getInstanceName())) {
+ manager.getClusterManagmentTool()
+ .enableInstance(manager.getClusterName(), manager.getInstanceName(), false,
+ InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
+ }
+ HelixEventHandlingUtil.updateCloudEventOperationInClusterConfig(manager.getClusterName(),
+ manager.getInstanceName(), manager.getHelixDataAccessor().getBaseDataAccessor(), false,
+ message);
}
/**
* Enable the instance
+ * We only enable instance that is disabled because of cloud event.
* @param manager The helix manager associated with the listener
* @param eventInfo Detailed information about the event
*/
public void enableInstance(HelixManager manager, Object eventInfo) {
- // To be implemented
- throw new NotImplementedException();
+ LOG.info("DefaultCloudEventCallbackImpl enable Instance {}", manager.getInstanceName());
+ String instanceName = manager.getInstanceName();
+ HelixDataAccessor accessor = manager.getHelixDataAccessor();
+ String message = String.format(_instanceReason, System.currentTimeMillis());
+ HelixEventHandlingUtil
+ .updateCloudEventOperationInClusterConfig(manager.getClusterName(), instanceName,
+ manager.getHelixDataAccessor().getBaseDataAccessor(), true, message);
+ if (HelixEventHandlingUtil.isInstanceDisabledForCloudEvent(instanceName, accessor)) {
+ manager.getClusterManagmentTool().enableInstance(manager.getClusterName(), instanceName, true,
+ InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
+ }
}
/**
- *
+ * Will enter MM when the cluster is not in MM
+ * TODO: we should add maintenance reason when EMM with cloud event
* @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())) {
+ LOG.info("DefaultCloudEventCallbackImpl enterMaintenanceMode by {}",
+ manager.getInstanceName());
+ manager.getClusterManagmentTool()
+ .manuallyEnableMaintenanceMode(manager.getClusterName(), true,
+ String.format(_emmReason, manager.getInstanceName(), System.currentTimeMillis()),
+ null);
+ }
}
/**
- *
+ * Will exit MM when when cluster config tracks no ongoing cloud event being handling
+ * TODO: we should also check the maintenance reason and only exit when EMM is caused by 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) {
- // To be implemented
- throw new NotImplementedException();
+ List<String> instances =
+ manager.getClusterManagmentTool().getInstancesInCluster(manager.getClusterName());
+ ClusterConfig clusterConfig = manager.getHelixDataAccessor()
+ .getProperty(manager.getHelixDataAccessor().keyBuilder().clusterConfig());
+ Map<String, String> clusterConfigTrackedEvent = clusterConfig.getDisabledInstancesWithInfo();
+ if (clusterConfigTrackedEvent == null || clusterConfigTrackedEvent.isEmpty()
+ || clusterConfigTrackedEvent.entrySet().stream().noneMatch(
+ e -> ConfigStringUtil.parseConcatenatedConfig(e.getValue())
+ .get(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_TYPE.toString())
+ .equals(InstanceConstants.InstanceDisabledType.CLOUD_EVENT.name()))) {
Review Comment:
Let's simplify the condition, or move it to a method, thanks.
##########
helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java:
##########
@@ -19,51 +19,109 @@
* under the License.
*/
+import java.util.List;
+import java.util.Map;
+
+import org.apache.helix.HelixDataAccessor;
import org.apache.helix.HelixManager;
-import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+import org.apache.helix.constants.InstanceConstants;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.util.ConfigStringUtil;
+import org.apache.helix.util.InstanceValidationUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
/**
* A default callback implementation class to be used in {@link HelixCloudEventListener}
*/
public class DefaultCloudEventCallbackImpl {
+ private static final Logger LOG = LoggerFactory.getLogger(DefaultCloudEventCallbackImpl.class);
+ private final String _instanceReason = "Cloud event in DefaultCloudEventCallback at %s";
+ private final String _emmReason = "Cloud event EMM in DefaultCloudEventCallback by %s at %s";
/**
- * Disable the instance
+ * Disable the instance and track the cloud event in map field disabledInstancesWithInfo in
+ * cluster config. Will not re-disable the instance if the instance is already disabled for
+ * other reason. (So we will not overwrite the disabled reason and enable this instance when
+ * on-unpause)
* @param manager The helix manager associated with the listener
* @param eventInfo Detailed information about the event
*/
public void disableInstance(HelixManager manager, Object eventInfo) {
- // To be implemented
- throw new NotImplementedException();
+ String message = String.format(_instanceReason, System.currentTimeMillis());
Review Comment:
A better way might be to wrap message generation in a static method, it's more self-explanative and we won't need to declare the string template
##########
helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java:
##########
@@ -19,51 +19,109 @@
* under the License.
*/
+import java.util.List;
+import java.util.Map;
+
+import org.apache.helix.HelixDataAccessor;
import org.apache.helix.HelixManager;
-import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+import org.apache.helix.constants.InstanceConstants;
+import org.apache.helix.model.ClusterConfig;
+import org.apache.helix.util.ConfigStringUtil;
+import org.apache.helix.util.InstanceValidationUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
/**
* A default callback implementation class to be used in {@link HelixCloudEventListener}
*/
public class DefaultCloudEventCallbackImpl {
+ private static final Logger LOG = LoggerFactory.getLogger(DefaultCloudEventCallbackImpl.class);
+ private final String _instanceReason = "Cloud event in DefaultCloudEventCallback at %s";
+ private final String _emmReason = "Cloud event EMM in DefaultCloudEventCallback by %s at %s";
/**
- * Disable the instance
+ * Disable the instance and track the cloud event in map field disabledInstancesWithInfo in
+ * cluster config. Will not re-disable the instance if the instance is already disabled for
+ * other reason. (So we will not overwrite the disabled reason and enable this instance when
+ * on-unpause)
* @param manager The helix manager associated with the listener
* @param eventInfo Detailed information about the event
*/
public void disableInstance(HelixManager manager, Object eventInfo) {
- // To be implemented
- throw new NotImplementedException();
+ String message = String.format(_instanceReason, System.currentTimeMillis());
+ LOG.info("DefaultCloudEventCallbackImpl disabled Instance {}", manager.getInstanceName());
+ if (InstanceValidationUtil
+ .isEnabled(manager.getHelixDataAccessor(), manager.getInstanceName())) {
+ manager.getClusterManagmentTool()
+ .enableInstance(manager.getClusterName(), manager.getInstanceName(), false,
+ InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
+ }
+ HelixEventHandlingUtil.updateCloudEventOperationInClusterConfig(manager.getClusterName(),
+ manager.getInstanceName(), manager.getHelixDataAccessor().getBaseDataAccessor(), false,
+ message);
}
/**
* Enable the instance
+ * We only enable instance that is disabled because of cloud event.
* @param manager The helix manager associated with the listener
* @param eventInfo Detailed information about the event
*/
public void enableInstance(HelixManager manager, Object eventInfo) {
- // To be implemented
- throw new NotImplementedException();
+ LOG.info("DefaultCloudEventCallbackImpl enable Instance {}", manager.getInstanceName());
+ String instanceName = manager.getInstanceName();
+ HelixDataAccessor accessor = manager.getHelixDataAccessor();
+ String message = String.format(_instanceReason, System.currentTimeMillis());
+ HelixEventHandlingUtil
+ .updateCloudEventOperationInClusterConfig(manager.getClusterName(), instanceName,
+ manager.getHelixDataAccessor().getBaseDataAccessor(), true, message);
+ if (HelixEventHandlingUtil.isInstanceDisabledForCloudEvent(instanceName, accessor)) {
+ manager.getClusterManagmentTool().enableInstance(manager.getClusterName(), instanceName, true,
+ InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message);
+ }
}
/**
- *
+ * Will enter MM when the cluster is not in MM
+ * TODO: we should add maintenance reason when EMM with cloud event
* @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())) {
+ LOG.info("DefaultCloudEventCallbackImpl enterMaintenanceMode by {}",
+ manager.getInstanceName());
+ manager.getClusterManagmentTool()
+ .manuallyEnableMaintenanceMode(manager.getClusterName(), true,
+ String.format(_emmReason, manager.getInstanceName(), System.currentTimeMillis()),
+ null);
+ }
}
/**
- *
+ * Will exit MM when when cluster config tracks no ongoing cloud event being handling
+ * TODO: we should also check the maintenance reason and only exit when EMM is caused by 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) {
- // To be implemented
- throw new NotImplementedException();
+ List<String> instances =
+ manager.getClusterManagmentTool().getInstancesInCluster(manager.getClusterName());
+ ClusterConfig clusterConfig = manager.getHelixDataAccessor()
+ .getProperty(manager.getHelixDataAccessor().keyBuilder().clusterConfig());
+ Map<String, String> clusterConfigTrackedEvent = clusterConfig.getDisabledInstancesWithInfo();
+ if (clusterConfigTrackedEvent == null || clusterConfigTrackedEvent.isEmpty()
+ || clusterConfigTrackedEvent.entrySet().stream().noneMatch(
+ e -> ConfigStringUtil.parseConcatenatedConfig(e.getValue())
+ .get(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_TYPE.toString())
+ .equals(InstanceConstants.InstanceDisabledType.CLOUD_EVENT.name()))) {
+ LOG.info("DefaultCloudEventCallbackImpl exitMaintenanceMode by {}",
+ manager.getInstanceName());
+ manager.getClusterManagmentTool()
+ .manuallyEnableMaintenanceMode(manager.getClusterName(), false,
+ String.format(_emmReason, manager.getInstanceName(), System.currentTimeMillis()),
+ null);
+ }
Review Comment:
Do we want to log something in the else part?
--
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