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 2020/09/28 19:22:32 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1413: Feature: Offline Node Timeout During Maintenance Mode
jiajunwang commented on a change in pull request #1413:
URL: https://github.com/apache/helix/pull/1413#discussion_r496140640
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -253,9 +254,34 @@ private void refreshIdealState(final HelixDataAccessor accessor,
private void refreshLiveInstances(final HelixDataAccessor accessor,
Set<HelixConstants.ChangeType> refreshedType) {
if (_propertyDataChangedMap.get(HelixConstants.ChangeType.LIVE_INSTANCE).getAndSet(false)) {
+ // Keep a copy of old live instances in case of maintenance mode
+ Map<String, LiveInstance> oldLiveInstances = getLiveInstances();
_liveInstanceCache.refresh(accessor);
_updateInstanceOfflineTime = true;
refreshedType.add(HelixConstants.ChangeType.LIVE_INSTANCE);
+
+ // If maintenance mode is enabled and timeout window is specified, filter 'new' live nodes
+ // for timed-out nodes
+ long timeOutWindow = _clusterConfig.getMaintenanceOfflineNodeTimeOut();
+ if (timeOutWindow >= 0 && isMaintenanceModeEnabled()) {
+ for (String instance : _liveInstanceCache.getPropertyMap().keySet()) {
+ // For every 'new' live node, check if it's timed-out
+ if (!oldLiveInstances.containsKey(instance) && isInstanceTimedOutDuringMaintenance(
Review comment:
If there is a leadership switch, the oldLiveInstances list will be empty. So the behavior would rely on isInstanceTimedOutDuringMaintenance() only. My guess is that we don't need oldLiveInstances list, since it won't be able to cover leadership switch case anyway.
If isInstanceTimedOutDuringMaintenance() has anything missed, so you have to relies on the oldLiveInstances list, then we need to fix it within the scope of isInstanceTimedOutDuringMaintenance().
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -728,6 +756,41 @@ private void updateDisabledInstances() {
}
}
+ /*
+ * Check if the instance is timed-out during maintenance mode. An instance is timed-out if it has
+ * been offline for longer than the user defined timeout window.
+ * @param timeOutWindow - the timeout window; guaranteed to be non-negative
+ */
+ private boolean isInstanceTimedOutDuringMaintenance(HelixDataAccessor accessor, String instance,
+ long timeOutWindow) {
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+ PropertyKey propertyKey = keyBuilder.participantHistory(instance);
+ ParticipantHistory history = accessor.getProperty(propertyKey);
+
+ // Cannot use _instanceOfflineTimeMap. _instanceOfflineTimeMap is last updated in the previous
+ // pipeline execution; it is possible a new offline timestamp is updated after the previous
+ // pipeline execution, so we need the most updated timestamp.
+ long lastOfflineTime = history.getLastTimeInOfflineHistory();
+ // lastOfflineTime is only negative if there is no offline history or the time format is wrong.
+ // Since this instance is a 'new' live instance, not having offline history = first time created
+ // instance; during maintenance mode, no partition will be assigned to such a new instance,
+ // therefore it's okay to no time it out. The wrong format case shouldn't happen at all and will
+ // not be handled either.
+ if (lastOfflineTime < 0) {
+ return false;
+ }
+
+ long currentTime = System.currentTimeMillis();
+ if (currentTime - lastOfflineTime > timeOutWindow) {
+ LogUtil.logWarn(logger, getClusterEventId(), String.format(
+ "During maintenance mode, instance %s is timed-out due to its offline time. Current time: "
Review comment:
nit, this calculation does not need to be bound with the maintenance mode, let's don't mention "maintenance mode" in the log.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -728,6 +756,41 @@ private void updateDisabledInstances() {
}
}
+ /*
+ * Check if the instance is timed-out during maintenance mode. An instance is timed-out if it has
+ * been offline for longer than the user defined timeout window.
+ * @param timeOutWindow - the timeout window; guaranteed to be non-negative
+ */
+ private boolean isInstanceTimedOutDuringMaintenance(HelixDataAccessor accessor, String instance,
+ long timeOutWindow) {
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+ PropertyKey propertyKey = keyBuilder.participantHistory(instance);
+ ParticipantHistory history = accessor.getProperty(propertyKey);
+
+ // Cannot use _instanceOfflineTimeMap. _instanceOfflineTimeMap is last updated in the previous
+ // pipeline execution; it is possible a new offline timestamp is updated after the previous
+ // pipeline execution, so we need the most updated timestamp.
+ long lastOfflineTime = history.getLastTimeInOfflineHistory();
+ // lastOfflineTime is only negative if there is no offline history or the time format is wrong.
+ // Since this instance is a 'new' live instance, not having offline history = first time created
+ // instance; during maintenance mode, no partition will be assigned to such a new instance,
+ // therefore it's okay to no time it out. The wrong format case shouldn't happen at all and will
+ // not be handled either.
+ if (lastOfflineTime < 0) {
Review comment:
Strictly check for ONLINE? Of course, keeping the < 0 checks for safe is still good here, but optional. I suggest adding "lastOfflineTime == ONLINE" to make the logic easier for the reader to understand.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -728,6 +756,41 @@ private void updateDisabledInstances() {
}
}
+ /*
+ * Check if the instance is timed-out during maintenance mode. An instance is timed-out if it has
+ * been offline for longer than the user defined timeout window.
+ * @param timeOutWindow - the timeout window; guaranteed to be non-negative
+ */
+ private boolean isInstanceTimedOutDuringMaintenance(HelixDataAccessor accessor, String instance,
+ long timeOutWindow) {
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+ PropertyKey propertyKey = keyBuilder.participantHistory(instance);
+ ParticipantHistory history = accessor.getProperty(propertyKey);
+
+ // Cannot use _instanceOfflineTimeMap. _instanceOfflineTimeMap is last updated in the previous
+ // pipeline execution; it is possible a new offline timestamp is updated after the previous
+ // pipeline execution, so we need the most updated timestamp.
+ long lastOfflineTime = history.getLastTimeInOfflineHistory();
Review comment:
There is a chance that we failed to record the offlinetime, or if pipeline runs slower, the offline time is not accurately recorded. I don't think there is a solution for it, but let's mention in the comment.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ParticipantHistory.java
##########
@@ -40,7 +41,9 @@
private static Logger LOG = LoggerFactory.getLogger(ParticipantHistory.class);
private final static int HISTORY_SIZE = 20;
- private enum ConfigProperty {
+ final static String HISTORY_DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss:SSS";
Review comment:
private?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -122,7 +122,13 @@
// don't specify their individual pool sizes, this value will be used for all participants; if
// none of participants or the cluster define pool sizes,
// TaskConstants.DEFAULT_TASK_THREAD_POOL_SIZE will be used to create pool sizes.
- GLOBAL_TARGET_TASK_THREAD_POOL_SIZE
+ GLOBAL_TARGET_TASK_THREAD_POOL_SIZE,
+
+ // The time out window for offline nodes during maintenance mode; if an offline node has been
+ // offline for more than this specified time period, it's treated as offline for the rest of
+ // the maintenance mode's duration even when it comes online.
+ // The unit is milliseconds.
+ MAINTENANCE_OFFLINE_NODE_TIME_OUT
Review comment:
nit, OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE?
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -298,6 +298,26 @@ public void testAsyncGlobalRebalanceOption() {
false), true);
}
+ @Test
+ public void testGetMaintenanceOfflineNodeTimeOut() {
Review comment:
1. We need a test to cover the get default value case.
2. Might be easier for you to combine the 2 new tests.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -728,6 +756,41 @@ private void updateDisabledInstances() {
}
}
+ /*
+ * Check if the instance is timed-out during maintenance mode. An instance is timed-out if it has
+ * been offline for longer than the user defined timeout window.
+ * @param timeOutWindow - the timeout window; guaranteed to be non-negative
+ */
+ private boolean isInstanceTimedOutDuringMaintenance(HelixDataAccessor accessor, String instance,
+ long timeOutWindow) {
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+ PropertyKey propertyKey = keyBuilder.participantHistory(instance);
+ ParticipantHistory history = accessor.getProperty(propertyKey);
+
+ // Cannot use _instanceOfflineTimeMap. _instanceOfflineTimeMap is last updated in the previous
+ // pipeline execution; it is possible a new offline timestamp is updated after the previous
+ // pipeline execution, so we need the most updated timestamp.
+ long lastOfflineTime = history.getLastTimeInOfflineHistory();
Review comment:
Due to the same reason, we might want to limit the timeout setup to be more than 5 mins (for example). A very small number does not make sense, and it won't work as expected anyway.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -253,9 +254,34 @@ private void refreshIdealState(final HelixDataAccessor accessor,
private void refreshLiveInstances(final HelixDataAccessor accessor,
Set<HelixConstants.ChangeType> refreshedType) {
if (_propertyDataChangedMap.get(HelixConstants.ChangeType.LIVE_INSTANCE).getAndSet(false)) {
+ // Keep a copy of old live instances in case of maintenance mode
+ Map<String, LiveInstance> oldLiveInstances = getLiveInstances();
_liveInstanceCache.refresh(accessor);
_updateInstanceOfflineTime = true;
refreshedType.add(HelixConstants.ChangeType.LIVE_INSTANCE);
+
+ // If maintenance mode is enabled and timeout window is specified, filter 'new' live nodes
+ // for timed-out nodes
+ long timeOutWindow = _clusterConfig.getMaintenanceOfflineNodeTimeOut();
+ if (timeOutWindow >= 0 && isMaintenanceModeEnabled()) {
+ for (String instance : _liveInstanceCache.getPropertyMap().keySet()) {
+ // For every 'new' live node, check if it's timed-out
+ if (!oldLiveInstances.containsKey(instance) && isInstanceTimedOutDuringMaintenance(
+ accessor, instance, timeOutWindow)) {
+ _timedOutInstanceDuringMaintenance.add(instance);
+ }
+ }
+
+ // Remove all timed-out nodes that were recorded in this maintenance duration
+ for (String instance : _timedOutInstanceDuringMaintenance) {
+ _liveInstanceCache.deletePropertyByName(instance);
Review comment:
It's better to ensure the cache has all the facts. Removing the timedout instances from the cache might be suboptimal since after this, no one tracking the real alive nodes anymore.
It would be easier to keep tracking the whole list and the timedout list, then if the filter option is turned on, we filter in the get method. In this way, we still have the capability to return the full list.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ParticipantHistory.java
##########
@@ -40,7 +41,9 @@
private static Logger LOG = LoggerFactory.getLogger(ParticipantHistory.class);
private final static int HISTORY_SIZE = 20;
- private enum ConfigProperty {
+ final static String HISTORY_DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss:SSS";
Review comment:
If required in the test, then either put it in HelixConstants or redefine a string in the test case (this helps to prevent if anyone changes the format that breaks the compatibility)
----------------------------------------------------------------
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.
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