dasahcc commented on a change in pull request #1000:
URL: https://github.com/apache/helix/pull/1000#discussion_r431401209
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -110,9 +112,12 @@
* define the next possible notification types
*/
private static Map<Type, List<Type>> nextNotificationType = new HashMap<>();
+
static {
- nextNotificationType.put(Type.INIT, Arrays.asList(Type.CALLBACK,
Type.FINALIZE));
- nextNotificationType.put(Type.CALLBACK, Arrays.asList(Type.CALLBACK,
Type.FINALIZE));
+ nextNotificationType
Review comment:
Is this agreed? I am not sure. I was preferring to have a separate type
to by pass even if there is a callback wrong order issue.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -292,6 +295,8 @@ public ZKHelixManager(String clusterName, String
instanceName, InstanceType inst
.getSystemPropertyAsInt(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY,
ParticipantHealthReportTask.DEFAULT_REPORT_LATENCY);
+ _messageRefreshInterval =
HelixUtil.getSystemPropertyAsLong(SystemPropertyKeys.MESSAGE_REFRESH_INTERVAL,
VALUE_NOT_SET);
Review comment:
Do we need a variable for that? I dont think we will reuse the value in
later stage.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -392,17 +450,18 @@ public void invoke(NotificationContext changeContext)
throws Exception {
logger.warn("Callback handler received event in wrong order. Listener:
" + _listener
Review comment:
My assumption is that you have a new type to by pass when there is a
callback type wrong order. But here you may still have the problem that current
type is FINALIZE and coming with CALLBACK. And your type PERODIC_REFRESH will
also be recognized as a wrong order type.
I would suggest rethink the value of adding the new type. What is the
purpose to have a new type.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -202,6 +214,29 @@ protected void handleEvent(NotificationContext event) {
}
}
+ class RefreshTask implements Runnable {
+ @Override
+ public void run() {
+ try {
+ long currentTime = System.currentTimeMillis();
+ if (_lastEventTime + _periodicRefreshInterval <= currentTime) {
+ NotificationContext changeContext = new
NotificationContext(_manager);
+ changeContext.setType(NotificationContext.Type.PERIODIC_REFRESH);
+ changeContext.setChangeType(_changeType);
+ enqueueTask(changeContext);
+ } else {
+ long remainingTime = _lastEventTime + _periodicRefreshInterval -
currentTime;
+ _scheduledRefreshFuture.cancel(false);
+ _scheduledRefreshFuture = _periodicRefreshExecutor
+ .scheduleWithFixedDelay(this, remainingTime,
_periodicRefreshInterval,
+ TimeUnit.MILLISECONDS);
Review comment:
There would be a good example of Delayed Scheduler. Try to understand
that mode.
----------------------------------------------------------------
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]
