[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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...
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...
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...
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...
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...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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...
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...
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...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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...
Github user lvfangmin commented on the issue: https://github.com/apache/zookeeper/pull/689 Thanks @tumativ, I'm reviewing this now. ---
[GitHub] zookeeper issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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...
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...
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...
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...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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 issue #689: ZOOKEEPER-3183:Notifying the WatcherCleaner thread and...
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/ ---
