[GitHub] [helix] jiajunwang commented on a change in pull request #1000: Add message periodic refresh

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1000:
URL: https://github.com/apache/helix/pull/1000#discussion_r432194951



##########
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:
       This logic seems to be incorrect... But I guess you will change it anyway.

##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -78,4 +78,10 @@
   // System Property Metadata Store Directory Server endpoint key
   public static final String MSDS_SERVER_ENDPOINT_KEY =
       MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
+
+  // Periodic message refresh interval, must be > 0
+  public static final String MESSAGE_REFRESH_INTERVAL = "helix.manager.messageRefreshInterval.ms";
+
+  // When system property is not set, use this as the value for getProperty
+  public static final int PROPERTY_NOT_SET = -1;

Review comment:
       Usually, we put this in the logic that uses the property. Not all properties are a number. And some of them can be -1 actually. This is not generic enough.

##########
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:
       Let's just use CALLBACK for the periodic triggering.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -818,4 +884,15 @@ public String getContent() {
         + ", _listener=" + _listener + ", _changeType=" + _changeType + ", _manager=" + _manager
         + ", _zkClient=" + _zkClient + '}';
   }
+
+  private void initRefreshTask() {
+    _lastEventTime = System.currentTimeMillis();
+    _periodicRefreshExecutor = new ScheduledThreadPoolExecutor(1);
+    // When cancelling the task future, it removes the task from the queue
+    // so we won't have a memory leakage when we cancel scheduled task
+    _periodicRefreshExecutor.setRemoveOnCancelPolicy(true);
+    _scheduledRefreshFuture = _periodicRefreshExecutor
+        .scheduleWithFixedDelay(new RefreshTask(), _periodicRefreshInterval,

Review comment:
       I think we talked about at least 2 of the following points:
   1. If the callback is triggered, the timer should be reset and the wait until the interval passes.
   2. If the queue is not empty, then we don't need to trigger the refresh.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -818,4 +884,15 @@ public String getContent() {
         + ", _listener=" + _listener + ", _changeType=" + _changeType + ", _manager=" + _manager
         + ", _zkClient=" + _zkClient + '}';
   }
+
+  private void initRefreshTask() {
+    _lastEventTime = System.currentTimeMillis();
+    _periodicRefreshExecutor = new ScheduledThreadPoolExecutor(1);
+    // When cancelling the task future, it removes the task from the queue
+    // so we won't have a memory leakage when we cancel scheduled task
+    _periodicRefreshExecutor.setRemoveOnCancelPolicy(true);
+    _scheduledRefreshFuture = _periodicRefreshExecutor
+        .scheduleWithFixedDelay(new RefreshTask(), _periodicRefreshInterval,

Review comment:
       They need to be done to avoid unnecessary performance impact.

##########
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:
       This PR is not addressing the wrong order problem, right? I don't think we need this change here.
   Even if we are going to do it, should be in a separate change.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -202,6 +211,30 @@ protected void handleEvent(NotificationContext event) {
     }
   }
 
+  class RefreshTask implements Runnable {
+    @Override
+    public void run() {
+      try {
+        long currentTime = System.currentTimeMillis();
+        long remainingTime = _lastEventTime + _periodicRefreshInterval - currentTime;
+        if (remainingTime <= 0) {

Review comment:
       What if there is already one periodic event in the queue? Shall we keep adding new ones?

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -131,6 +136,10 @@
   private boolean _batchModeEnabled = false;
   private boolean _preFetchEnabled = true;
   private HelixCallbackMonitor _monitor;
+  private final long _periodicRefreshInterval;

Review comment:
       For generosity, shall we call it periodicCallbackInterval, or periodicTriggerInterval?




----------------------------------------------------------------
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