You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by tumativ <gi...@git.apache.org> on 2018/11/05 11:56:51 UTC
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
GitHub user tumativ opened a pull request:
https://github.com/apache/zookeeper/pull/689
ZOOKEEPER-3183:Notifying the WatcherCleaner thread and Threads which …
…are waiting list to add watchers during the shutdown and avoid adding the dead watchers when shut down is initiated
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tumativ/zookeeper ZOOKEEPER-3183
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/689.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #689
----
commit c6e0f7af5aace94daf0caf0ba7b860e99540a284
Author: vtumati <vt...@...>
Date: 2018-11-05T11:51:23Z
ZOOKEEPER-3183:Notifying the WatcherCleaner thread and Threads which are waiting list to add watchers during shutdown and avoid adding the dead watchers when shut down is initiated
----
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@lvfangmin
- Regarding adding a break in addDeadWatcher.We basically do not interrupt the caller thread of addWatcher during the shutdown. So the caller thread will not be interrupted during the shutdown.
-The caller thread of addWatcher can be interrupted out side of the shutdown. Do you recommend break and adding watcher even it reaches max pending watchers or return without adding watcher?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
Thanks @tumativ, I'm reviewing this now.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236063217
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,24 +104,24 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
- }
- } catch (InterruptedException e) {
- LOG.info("Got interrupted while waiting for dead watches " +
- "queue size");
- }
- }
- synchronized (this) {
- if (deadWatchers.add(watcherBit)) {
- totalDeadWatchers.incrementAndGet();
- if (deadWatchers.size() >= watcherCleanThreshold) {
- synchronized (cleanEvent) {
- cleanEvent.notifyAll();
- }
- }
- }
+ synchronized (processingCompletedEvent) {
--- End diff --
We don't have a general format wiki yet, we'll work on that, in general it's 4 white space after { line.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
I realized that processing remaining watchers is not a good idea without knowing the context of the shutdown. The shutdown contract itself is stop processing. I have modified the JIRA description.
It targets below improvements.
- Notify the WatcherCleaner thread during shutdown if it is waiting for dead watchers get the certain number(watcherCleanThreshold)
- Notify the threads who are waiting on totalDeadWatchers, this can happen when pending watchers are reached maximum
-Stop taking incoming dead watcher and adding to deadWatchers even cleaner thread is stopped.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ There is a very short window that we'll still add the dead watcher to the cleaner thread, I don't expect that will add too much GC overhead for these small amount of dead watch bits.
For your 2nd point, you can interrupt and wait for this thread to exit using join, although I'm not sure it's worth to, give these clean up could be done in background without affecting us starting a new ZK server with new ZKDatabase.
So I still prefer to simply interrupt instead of this change, both from complexity (which also means error-prone and hard to maintain in the future) and efficient sacrificed here.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236063610
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,12 +103,13 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
+ synchronized(processingCompletedEvent) {
+ processingCompletedEvent.wait(100);
--- End diff --
Done
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@anmolnar @lvfangmin can I ask you merge PR if there are no review comments?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/689
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2582/
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236102347
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,12 +103,13 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
+ synchronized(processingCompletedEvent) {
+ processingCompletedEvent.wait(100);
}
} catch (InterruptedException e) {
- LOG.info("Got interrupted while waiting for dead watches " +
+ LOG.info("Got interrupted while waiting for dead watchers " +
--- End diff --
Done
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234494972
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,24 +104,24 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
- }
- } catch (InterruptedException e) {
- LOG.info("Got interrupted while waiting for dead watches " +
- "queue size");
- }
- }
- synchronized (this) {
- if (deadWatchers.add(watcherBit)) {
- totalDeadWatchers.incrementAndGet();
- if (deadWatchers.size() >= watcherCleanThreshold) {
- synchronized (cleanEvent) {
- cleanEvent.notifyAll();
- }
- }
- }
+ synchronized (processingCompletedEvent) {
--- End diff --
@tumativ looks like we still have some indent problem for this patch, can you help correct those?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
@anmolnar that's a good point, I think the reason we didn't use ZooKeeperThread or ZooKeeperCriticalThread is because we don't expect to exit abnormally from the WatcherCleaner thread, which is true for now. But I think it's better to use ZooKeeperThread or even ZooKeeperCriticalThread to cover future changes which might cause the thread exit abnormally.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@anmolnar can I ask you merge pr if there is no review comments?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/689
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2581/
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234403902
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -177,6 +180,7 @@ public void shutdown() {
stopped = true;
deadWatchers.clear();
cleaners.stop();
+ super.interrupt();
--- End diff --
Done
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
Merged, thanks @tumativ. Btw, what's your Jira user id, I tried to assign this JIRA to you but I cannot find you.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/689
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@anmolnar and @lvfangmin Thanks for reviewing the changes.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ I tend to agree with @lvfangmin in adding `interrupt()` call to shutdown would be the easiest solution here.
In which case I would also add `break;` to the InterruptedException handler of addDeadWatcher() method.
Out of interest, why WatcherCleaner is not inherited from `ZooKeeperThread` btw?
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234403883
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -177,6 +180,7 @@ public void shutdown() {
stopped = true;
deadWatchers.clear();
cleaners.stop();
+ super.interrupt();
--- End diff --
Sure.Used super for better readability.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236098818
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,12 +103,13 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
+ synchronized(processingCompletedEvent) {
+ processingCompletedEvent.wait(100);
}
} catch (InterruptedException e) {
- LOG.info("Got interrupted while waiting for dead watches " +
+ LOG.info("Got interrupted while waiting for dead watchers " +
--- End diff --
Ditto, watches are more accurate.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236063177
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -163,8 +165,8 @@ public void doWork() throws Exception {
long latency = Time.currentElapsedTime() - startTime;
LOG.info("Takes {} to process {} watches", latency, total);
totalDeadWatchers.addAndGet(-total);
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.notifyAll();
+ synchronized(processingCompletedEvent) {
+ processingCompletedEvent.notifyAll();
--- End diff --
ditto.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234399479
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -177,6 +180,7 @@ public void shutdown() {
stopped = true;
deadWatchers.clear();
cleaners.stop();
+ super.interrupt();
--- End diff --
Maybe just change it to this.interrupt(), since we're not overwriting it.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234399392
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,24 +104,25 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
+ synchronized(produserAndConsumerLock) {
+ produserAndConsumerLock.wait(100);
}
} catch (InterruptedException e) {
LOG.info("Got interrupted while waiting for dead watches " +
"queue size");
+ break;
}
}
- synchronized (this) {
- if (deadWatchers.add(watcherBit)) {
- totalDeadWatchers.incrementAndGet();
- if (deadWatchers.size() >= watcherCleanThreshold) {
- synchronized (cleanEvent) {
- cleanEvent.notifyAll();
+ synchronized (this) {
--- End diff --
The indent seems not correct.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ I'll merge it once the build is green. I know this is blocked by an issue on master, but we should all feel the pain and resolve it asap.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ Only 2 days passed, let's give some chance for the community to review your code.
I'm particularly interested in @lvfangmin 's opinion.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ Not related to your changes. Apparently it has been introduced on the master branch recently.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r231718078
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -50,6 +50,8 @@
private volatile boolean stopped = false;
private final Object cleanEvent = new Object();
+ private final Object produserAndConsumerLock = new Object();
--- End diff --
The producer of the dead watcher is trying to get the lock on totalDeadWatchers when totalDeadWatchers reaches the maximum. The expectation is the consumer needs to reduce this count .totalDeadWatchers is AtomicInteger and getting a lock on AtomicInteger is a not a good idea as it has their own concurrency control mechanisms. It will basically confuse why the lock is required on totalDeadWatchers at producer level when we are expecting consumer should reduce this count and it has their own concurrency control mechanisms
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@asfgit Thanks for reviewing.
- I see that there is a gap in API contract that allowing the dead watcher even cleaner thread is not running. I hope it is the burden on GC even it is GCed.
- We can interrupt the cleaner thread, but It is fine if it is waiting for minimum threshold dead watchers at the time of interrupting. The thread is marked as interrupted if it is not really waiting and the exception will be thrown any subsequent wait. I think the prediction is important at least in thread communication.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236098795
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -161,10 +163,10 @@ public void doWork() throws Exception {
long startTime = Time.currentElapsedTime();
listener.processDeadWatchers(snapshot);
long latency = Time.currentElapsedTime() - startTime;
- LOG.info("Takes {} to process {} watches", latency, total);
+ LOG.info("Takes {} to process {} watchers", latency, total);
--- End diff --
Watches seems to be more reasonable here, watcher maps to a single client session, and it can watch multiple paths, so have multiple watches on a single watcher, the total value is the total watches count not watcher.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236063614
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -163,8 +165,8 @@ public void doWork() throws Exception {
long latency = Time.currentElapsedTime() - startTime;
LOG.info("Takes {} to process {} watches", latency, total);
totalDeadWatchers.addAndGet(-total);
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.notifyAll();
+ synchronized(processingCompletedEvent) {
+ processingCompletedEvent.notifyAll();
--- End diff --
Done
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234504039
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,24 +104,24 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
- }
- } catch (InterruptedException e) {
- LOG.info("Got interrupted while waiting for dead watches " +
- "queue size");
- }
- }
- synchronized (this) {
- if (deadWatchers.add(watcherBit)) {
- totalDeadWatchers.incrementAndGet();
- if (deadWatchers.size() >= watcherCleanThreshold) {
- synchronized (cleanEvent) {
- cleanEvent.notifyAll();
- }
- }
- }
+ synchronized (processingCompletedEvent) {
--- End diff --
Done. Can you please share formatter which is meant for this project? It will be useful in the future.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ I understand the intention of this JIRA, but to me looks like adding this.interrupt() in shutdown() should solve those problems.
We may add a dead watch to the cleaner after this, but it doesn't matter, this this watch cleaner won't be referenced anymore after shutdown, and it will GCed.
So why not just do this instead of the complexity changes here? Did I miss anything?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@lvfangmin Thanks for merging. My JIRA user id is Tumati.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/689
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2607/
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
I see find bug issues with the master branch. It is leading to failures of pre-commit builds. Here is the ref for failures.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r236063171
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,12 +103,13 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
+ synchronized(processingCompletedEvent) {
+ processingCompletedEvent.wait(100);
--- End diff --
Can we change this with 4 extra white spaces relative to the synchronized statement?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@lvfangmin Any more concerns?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ This patch makes sense to me and looks like a nice improvement.
In the Jira you're saying "also complete the remaining dead watchers when interrupt happen", but I cannot see the change related to this. If cleanUp thread gets interrupted it brakes the while loop and doesn't process `deadWatchers`. (which I believe is right, but not sure what you're referring to)
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234403894
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -102,24 +104,25 @@ public void addDeadWatcher(int watcherBit) {
totalDeadWatchers.get() >= maxInProcessingDeadWatchers) {
try {
RATE_LOGGER.rateLimitLog("Waiting for dead watchers cleaning");
- synchronized(totalDeadWatchers) {
- totalDeadWatchers.wait(100);
+ synchronized(produserAndConsumerLock) {
+ produserAndConsumerLock.wait(100);
}
} catch (InterruptedException e) {
LOG.info("Got interrupted while waiting for dead watches " +
"queue size");
+ break;
}
}
- synchronized (this) {
- if (deadWatchers.add(watcherBit)) {
- totalDeadWatchers.incrementAndGet();
- if (deadWatchers.size() >= watcherCleanThreshold) {
- synchronized (cleanEvent) {
- cleanEvent.notifyAll();
+ synchronized (this) {
--- End diff --
Done
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234399372
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -50,6 +50,8 @@
private volatile boolean stopped = false;
private final Object cleanEvent = new Object();
+ private final Object produserAndConsumerLock = new Object();
--- End diff --
Can you give it a more explicit name? produserAndConsumerLock seems too general here.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@Ivfangmin any more changes?
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
@tumativ thanks for working on this, only a minor comment now, I'll merge this when you updated this.
---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
LGTM, +1
Thanks @tumativ for all your effort on this, I'll commit this today.
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r231610533
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -50,6 +50,8 @@
private volatile boolean stopped = false;
private final Object cleanEvent = new Object();
+ private final Object produserAndConsumerLock = new Object();
--- End diff --
Why do you need this explicit lock object instead if `totalDeadWatchers`?
---
[GitHub] zookeeper pull request #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thr...
Posted by tumativ <gi...@git.apache.org>.
Github user tumativ commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/689#discussion_r234415866
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java ---
@@ -50,6 +50,8 @@
private volatile boolean stopped = false;
private final Object cleanEvent = new Object();
+ private final Object produserAndConsumerLock = new Object();
--- End diff --
done
---