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 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 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 user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@Ivfangmin any more changes?
---
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 user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/689
@lvfangmin Any more concerns?
---
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 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 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 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 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
Github user tumativ commented on the issue:
https://github.com/apache/zookeeper/pull/689
@anmolnar and @lvfangmin Thanks for reviewing the changes.
---
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
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
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.
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.
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
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/689
Thanks @tumativ, I'm reviewing this now.
---
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 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 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 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
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
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 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/
---
25 matches
Mail list logo
