Add message periodic refresh by mgao0 · Pull Request #1000 · apache/helix

mgao0 · 2020-05-11T17:09:56Z

Issues

My PR addresses the following Helix issues and references them in the PR description:

Description

Here are some details about my PR, including screenshots of any UI changes:

This PR adds a scheduled periodic task to read leftover messages from zk.
There are several situations that we would need this periodic refresh:

A read from ZK fails and there is no more event to trigger another read
Watcher is not registered properly so it fails to trigger the callback
The message is not processed correctly and is discarded but no more incoming events to trigger another read

What would not be fixed by this PR is out of order problem, which is another issue may cause missing messages. For example, we may miss an event because its callback comes in after a finalize type event, so it will be ignored. Since this periodic refresh is done through generating an event, this out of order issue may affect refresh events too.

The refresh is done in callback handler and will be triggered if the time has passed for as long as a user defined interval since last processed event. The refresh is achieved by queuing an empty callback type event into the event processor.

Tests

The following tests are written for this issue:

TestPerioidicRefresh

The following is the result of the "mvn test" command on the appropriate module:
Running

Commits

My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation (Optional)

In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Code Quality

My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)

jiajunwang

I see you have 2 similar PRs. And I think we are all reviewing the other one. Could you please specify which one is the right PR to review?
And you might need to refer to the comments there for this PR's change, unfortunately.

helix-core/src/main/java/org/apache/helix/messaging/DefaultMessagingService.java

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java

mgao0 · 2020-05-13T18:25:07Z

@jiajunwang 14 hours ago Contributor
Why do you need to call it here synchronously? I think it's simpler if we just put this event into the event queue.
Given this saying, this logic will be fitting better if it is in the HelixManager level. What I'm trying to propose if the HelixManger is initialized as a Participant, then we start this timer thread there. It just injects message event with a certain interval. Of course, if a real message event comes earlier, then we reset the timer.

Put this logic in the executor does not make sense to me.

mgao0 · 2020-05-13T18:25:47Z

@jiajunwang Sorry if it's confusing. This PR is for the periodic refresh. I have copied and pasted all your comments here and I will address them here.

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java

helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java

helix-core/src/main/java/org/apache/helix/messaging/DefaultMessagingService.java

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java

helix-core/src/test/java/org/apache/helix/messaging/handling/TestPerioidicMessageRefresh.java

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java

helix-core/src/test/java/org/apache/helix/messaging/handling/TestPerioidicMessageRefresh.java

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

kaisun2000 · 2020-05-28T06:28:06Z

I just saw this diff. In fact, this is a totally different from what it was before. I have quite some reservation to add periodical refresh in CallbackHandler.

CallbackHandler is the interface with zkclient managing the lifecycle of a specific Helix property path. Specifically they handle notification (further calling back to application logic) and registering further notification logic.

The part actually already had some notable issues (lose notification, or installing "leaked path etc) that should be addressed. We should not complicate this part with further here.

mgao0 · 2020-05-28T18:45:50Z

I just saw this diff. In fact, this is a totally different from what it was before. I have quite some reservation to add periodical refresh in CallbackHandler.

CallbackHandler is the interface with zkclient managing the lifecycle of a specific Helix property path. Specifically they handle notification (further calling back to application logic) and registering further notification logic.

The part actually already had some notable issues (lose notification, or installing "leaked path etc) that should be addressed. We should not complicate this part with further here.

Doing periodic refresh in callback handler can actually resolve the issue that you mentioned such as lost notification or missing watcher. This periodic refresh logic is very independent, it's a separate thread, and it put an event in the event queue only if there is no event processed for a while. That being said, it the callback handler is functioning well and events are processed normally, it won't add much burden to the performance since the periodic refresh event won't be generated.
Is there any specific concern you have on the periodic refresh?

helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2020-05-29T00:37:05Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

+    // so we won't have a memory leakage when we cancel scheduled task
+    _periodicRefreshExecutor.setRemoveOnCancelPolicy(true);
+    _scheduledRefreshFuture = _periodicRefreshExecutor
+        .scheduleWithFixedDelay(new RefreshTask(), _periodicRefreshInterval,


I think we talked about at least 2 of the following points:

If the callback is triggered, the timer should be reset and the wait until the interval passes.

If the queue is not empty, then we don't need to trigger the refresh.

They need to be done to avoid unnecessary performance impact.

@jiajunwang The previous PR contains the first condition you mentioned. I added the second condition in the updated PR. And both checks are still being done in the callback handler instead of ZK event queue.
As per our discussion on Friday, I tried to do this in the ZKEvent queue, and found it hard to do, when an event coming in at the ZKEvent level, we don't know the change type of the event, thus not able to reset the timer accordingly. Currently at the ZKEvent level, no Helix logics are involved so I think it's better we keep it that way.
For your concern on parallel invoke, we synchronize on Helix manager in the invoke method, so it should be fine.
Let's discuss more if you still have concern about this.

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java

jiajunwang · 2020-06-04T04:55:11Z

helix-core/src/test/java/org/apache/helix/TestPeriodicRefresh.java

+/**
+ * This class tests that if there are no incoming events, the onMessage method in message listener will be called by message periodic refresh
+ */
+public class TestPeriodicRefresh extends ZkUnitTestBase {


@kaisun2000 since you raise this concern, please review this test case and verify if this is enough or not : )

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2020-06-04T18:13:48Z

Thanks for the update. Could you please add more information in the description about what the issue this refresh trying to address? I mean we discussed several scenarios that cannot be addressed by the other fixes, then we decided to proceed to finish this PR. But let's make it clear what is the missing part.

jiajunwang

"Refresh" to "Trigger" for all the parameters for consistency?
Or we can call it "periodicCallbackInterval"? Then scheduleCallbackFuture, etc.

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2020-06-09T22:17:48Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

+            wait(remainingTime);
+          }
+        }
+      } catch (Exception e) {


For the interruption exception, we may want to exit the execution.

I have made changes to separate the interrupted exception with other exceptions. I will interrupt the thread when interrupted exception is caught, is this what you wanted?

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2020-06-09T22:23:28Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

            _batchCallbackProcessor.resetEventQueue();
+            if (_scheduledRefreshFuture != null) {
+              _periodicRefreshExecutor.shutdownNow();
+              initRefreshTask();


The next event would be FINALIZE, so even the periodic task is triggered, it won't really invoke the handling logic successfully. I think we shall not to init here.

Shall we do it in the init() call?

I have updated the initTriggerTask() method to include reset as well. Please take a look. Thanks!

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java

jiajunwang

Looks good in general. Some minor comments.
Meanwhile, I think we are still discussing the default configuration. Let me approve after that decision has been made.

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2020-06-10T18:38:57Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

+    }
+    // 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
+    _periodicTriggerExecutor.setRemoveOnCancelPolicy(true);


Shall we only set it once after the fresh construct?

Good point. Updated.

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2020-06-11T19:12:13Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

        subscribeForChangesAsyn(changeContext.getType(), _path, _watchChild);
      }

+      if (_periodicTriggerExecutor != null) {


Can we put this in the initTriggerTask() method?

This is to prevent the update of last invoke time for the cases where periodic trigger is not enabled. I don't think I can move this to init?

Let's put it to the very beginning of this method?

jiajunwang · 2020-06-11T19:15:21Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

+   * Used to initialize a periodic triggered task
+   * Schedule tasks in a task executor with fixed intervals
+   */
+  private void initTriggerTask() {


nit, take _periodicTriggerInterval as the parameter, and don't refer to the global private field. This is a good style in which you can reduce the impact of a private field in the class. So less dependency if you want to make any further change.

Great suggestion! Fixed. Thanks!

jiajunwang · 2020-06-11T19:18:22Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

+      _periodicTriggerExecutor.setRemoveOnCancelPolicy(true);
+    }
+    _scheduledTriggerFuture = _periodicTriggerExecutor
+        .scheduleWithFixedDelay(new TriggerTask(), _periodicTriggerInterval,


I overlooked this part before. Why do you need to use scheduleWithFixedDelay() here? The TriggerTask object will keep running in the thread, right?

Discussed offline. A single long running thread is sufficient. Updated.

jiajunwang · 2020-06-11T19:19:31Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

+      try {
+        while (true) {
+          long currentTime = System.currentTimeMillis();
+          long remainingTime = _lastInvokeTime + _periodicTriggerInterval - currentTime;


To reduce the usage of _periodicTriggerInterval in the whole callback class, could you please pass the interval as a constructor parameter? That would be a cleaner design.

helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java

jiajunwang · 2020-06-11T22:08:00Z

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

        subscribeForChangesAsyn(changeContext.getType(), _path, _watchChild);
      }

+      if (_periodicTriggerExecutor != null) {


Let's put it to the very beginning of this method?

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java

jiajunwang · 2021-05-07T18:13:01Z

Close due to inactive.

mgao0 changed the title ~~[WIP] Add message periodic refresh~~ Add message periodic refresh May 12, 2020

mgao0 marked this pull request as ready for review May 12, 2020 21:59

jiajunwang reviewed May 13, 2020

View reviewed changes

mgao0 commented May 13, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/messaging/DefaultMessagingService.java Outdated Show resolved Hide resolved

mgao0 commented May 13, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java Outdated Show resolved Hide resolved

mgao0 commented May 13, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java Outdated Show resolved Hide resolved

mgao0 mentioned this pull request May 13, 2020

Use updaters to update read messages to ZK #1002

Merged

7 tasks

kaisun2000 reviewed May 14, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java Outdated Show resolved Hide resolved

huizhilu reviewed May 14, 2020

View reviewed changes

kaisun2000 suggested changes May 14, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java Outdated Show resolved Hide resolved

narendly reviewed May 18, 2020

View reviewed changes

mgao0 force-pushed the master branch 2 times, most recently from d4c167f to 00cb800 Compare May 26, 2020 23:12

NealSun96 reviewed May 26, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java Show resolved Hide resolved

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java Outdated Show resolved Hide resolved

NealSun96 reviewed May 26, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java Outdated Show resolved Hide resolved

NealSun96 requested changes May 27, 2020

View reviewed changes

helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java Outdated Show resolved Hide resolved

junkaixue reviewed May 27, 2020

View reviewed changes

mgao0 force-pushed the master branch from 492dda4 to b7ded4f Compare May 28, 2020 18:49

jiajunwang reviewed May 29, 2020

View reviewed changes

mgao0 force-pushed the master branch from b7ded4f to 3cf1ae1 Compare June 1, 2020 23:25

jiajunwang reviewed Jun 4, 2020

View reviewed changes

jiajunwang reviewed Jun 9, 2020

View reviewed changes

mgao0 force-pushed the master branch from 5eb522c to 7beda7f Compare June 10, 2020 17:43

jiajunwang reviewed Jun 10, 2020

View reviewed changes

mgao0 force-pushed the master branch from d6949fa to f3aeba7 Compare June 11, 2020 18:59

jiajunwang reviewed Jun 11, 2020

View reviewed changes

jiajunwang approved these changes Jun 11, 2020

View reviewed changes

mgao0 force-pushed the master branch from 5b8c28c to a08ee89 Compare June 12, 2020 17:22

mgao0 force-pushed the master branch 2 times, most recently from 5d36f68 to bb9bd30 Compare June 25, 2020 23:28

jiajunwang reviewed Jun 26, 2020

View reviewed changes

mgao0 added 20 commits August 13, 2020 12:38

Add message periodic refresh in callback handler

ecceec1

Change -1 to VALUE_NOT_SET

0110f97

Change reset method and put thread to sleep for early task execution

2d9addd

Chang task execution for when task is not due and add test

4ae938f

Change notification type ordering

016ce32

Change task scheduling

a1250d9

Add check on queue size

7121506

Default enable periodic refresh

ee86cb5

Fixed comments

5ec31ff

Add check for conditions when not to update last event time

36849e4

Improve parameter naming consistency and reset method

ed51fce

Update reset and init

32da1d8

Update reset and init method

fde868f

Change to Executor service

e2a066b

Move update invoke time

490850d

Some fixes

fa2177d

Change executor to static

f6d4aa7

Change trigger task to one time trigger

f1cb9fe

Queue event into batch callback processor

a4709b2

Move init and shut down to inside synchronized

0412e9e

mgao0 force-pushed the master branch from 9c0cfd3 to 0412e9e Compare August 13, 2020 21:20

jiajunwang closed this May 7, 2021

Conversation

mgao0 commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Description

Tests

Commits

Documentation (Optional)

Code Quality

Uh oh!

jiajunwang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgao0 commented May 13, 2020

Uh oh!

mgao0 commented May 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaisun2000 commented May 28, 2020

Uh oh!

mgao0 commented May 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jiajunwang commented Jun 4, 2020

Uh oh!

jiajunwang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mgao0 commented May 11, 2020 •

edited

Loading