jiajunwang commented on a change in pull request #1000:
URL: https://github.com/apache/helix/pull/1000#discussion_r445906416
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -132,6 +136,18 @@
private boolean _batchModeEnabled = false;
private boolean _preFetchEnabled = true;
private HelixCallbackMonitor _monitor;
+ private final long _periodicTriggerInterval;
+ private final static ExecutorService _periodicTriggerExecutor =
Review comment:
the static final field follows different name conventions.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -132,6 +136,18 @@
private boolean _batchModeEnabled = false;
private boolean _preFetchEnabled = true;
private HelixCallbackMonitor _monitor;
+ private final long _periodicTriggerInterval;
+ private final static ExecutorService _periodicTriggerExecutor =
+ Executors.newFixedThreadPool(40, new ThreadFactory() {
+ public Thread newThread(Runnable runnable) {
+ Thread thread = Executors.defaultThreadFactory().newThread(runnable);
+ thread.setDaemon(true);
+ thread.setName("TriggerTaskThread");
Review comment:
PeriodicTriggerTask. thread is not necessary, IMHO.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -201,6 +217,58 @@ protected void handleEvent(NotificationContext event) {
logger.warn("Exception in callback processing thread. Skipping
callback", e);
}
}
+
+ public boolean queueIsEmpty() {
+ return _eventQueue.isEmpty();
+ }
+ }
+
+ /**
+ * This class represents each periodic refresh task
+ * If the deadline passed, enqueue an event to do a refresh
+ * If the deadline has not passed (some events happened in between so don't
need to do a refresh),
+ * wait until reaching the interval and check again
+ */
+ class TriggerTask implements Runnable {
+ private final long _triggerInterval;
+
+ public TriggerTask(long triggerInterval) {
+ this._triggerInterval = triggerInterval;
+ }
+
+ @Override
+ public void run() {
+ logger.info("Starting trigger task.");
+ while (!Thread.currentThread().isInterrupted()) {
Review comment:
As we discussed this may cause the runnable to occupy the thread
forever, let's try the schedule executor back.
Unfortunately, the simpler design fails the tests. I hope the older design
will work.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -352,6 +434,10 @@ public String getPath() {
}
public void enqueueTask(NotificationContext changeContext) throws Exception {
+ if (_periodicTriggerExecutor != null) {
Review comment:
This check is no longer valid. We should use the interval or just use a
boolean (_periodicTriggerEnabled) so it is clearer
----------------------------------------------------------------
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]
