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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
