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/05/27 03:25:11 UTC
[GitHub] [helix] NealSun96 commented on a change in pull request #1000: Add message periodic refresh
NealSun96 commented on a change in pull request #1000:
URL: https://github.com/apache/helix/pull/1000#discussion_r430758536
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -781,8 +841,15 @@ void reset(boolean isShutdown) {
if (isShutdown) {
_batchCallbackProcessor.shutdown();
_batchCallbackProcessor = null;
+ if (_periodicRefreshExecutor != null) {
+ _periodicRefreshExecutor.shutdownNow();
+ }
} else {
_batchCallbackProcessor.resetEventQueue();
+ if (_periodicRefreshExecutor != null) {
+ _periodicRefreshExecutor.shutdownNow();
Review comment:
Just making sure: this line automatically cancels the scheduled runnable in `_scheduledRefreshFuture` right? If the refresh period is right after this line and before the next line, it's okay?
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -210,12 +245,25 @@ protected void handleEvent(NotificationContext event) {
public CallbackHandler(HelixManager manager, RealmAwareZkClient client, PropertyKey propertyKey,
Object listener, EventType[] eventTypes, ChangeType changeType) {
- this(manager, client, propertyKey, listener, eventTypes, changeType, null);
+ this(manager, client, propertyKey, listener, eventTypes, changeType, null, -1);
Review comment:
Should it use `VALUE_NOT_SET` here instead of "-1"?
##########
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:
Instead of doing this, can you just "sleep" for `remainingTime`, then do the check again? I think that automatically adjusts the schedule.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -202,6 +212,26 @@ 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;
+ Thread.sleep(remainingTime);
Review comment:
You need to do the refresh check again here, otherwise a whole new period/cycle will pass without refresh. :)
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -202,6 +212,26 @@ 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;
+ Thread.sleep(remainingTime);
Review comment:
You need to do the refresh check again here, otherwise a whole new period/cycle will pass without refresh. :) So it's more of a `while (there is remaining) { sleep }` type of structure.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -202,6 +212,26 @@ 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;
+ Thread.sleep(remainingTime);
Review comment:
Ok. I personally liked this approach because it's cleaner, but I'll let you be the judge of that.
----------------------------------------------------------------
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