Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/590
merged to master. great work @lvfangmin !
---
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/590
got a ["green"
build](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2285/testReport/)
(minors the known failed test `testReconfigRemoveClientFromStatic` discussed
at
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/590
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2278/
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Added more comments about the locking where @anmolnar and @hanm asked
during review, it will make the future reference easier as well. Also corrected
the typo and unused import.
---
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/590
thanks @lvfangmin for detailed reply.
just had another review pass over some files i forgot to look last time.
overall looks good. will sign this off once all review comments are addressed.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
@anmolnar #612 was fired because there is user reporting that issue, and
it's better to solve it earlier than waiting this diff being reviewed and
merged. I'll do the rebase to get rid of that
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/590
@anmolnar I'll sign this off by end of next Monday if no other issues.
@lvfangmin great work and thanks for your patience!
---
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/590
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2223/
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/590
I cannot commit, because we still have -1 from @hanm . Waiting for him to
approve.
Does it make #612 redundant?
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
@anmolnar Thanks for reviewing, take your time.
Here are the main differences between this version and the old one on the
Jira:
1. use path to watchers map instead of watcher
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/590
@lvfangmin Just to wrap up the difference between this and original
6-year-old patch on Jira: you've added `deadWatchers` collection and lazy
`WatcherCleaner` to avoid the performance penalty on
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/590
@lvfangmin Thanks. Sorry for the delay. I'd like to check one more thing
before accepting. Bear with me please.
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Rebased to resolve conflict. @anmolnar @hanm @maoling @nkalmar please
revisit this PR when you have time.
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Resolve conflict with latest code on master.
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Update based on the comments:
1. add findbugs exclusion
2. add comments for the newly added classes
3. add admin doc for the new JVM options introduced in this diff
4. move
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/590
systest will go to src/test/java , from my side, you can put the bench in
org.apache.zookeeper.test.system . Thinking about it, that's a pretty good
place.
No need to create main
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Thanks @nkalmar for the suggestion of bench project position, I was
following the same directory as the src/java/systest for now, do you think we
can move them together later? I'm not against
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/590
@lvfangmin Docs for the new caching parameter `zookeeper.bitHashCacheSize`?
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Added JMH micro benchmark for the watch manager:
* It shows **big win** for the watch heavy cases, with the current
implementation, it uses more than 50MB memory to store 1M watches,
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/590
This is going to be a huge improvement for Accumulo for example that is a
heavy user of watchers. I'm going to allocate some capacity to review these new
patches.
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
There is a PR in the Jira few years ago, which uses BitMap as well, but it
sacrificed the performance on triggering watches, this patch improved that and
uses lazy clean up for cleaning those
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
Based on internal benchmark, this may save more than 95% memory usage on
concentrate watches scenario. I'm working on adding some micro benchmark.
In the original Jira, @phunt also
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/590
@lvfangmin interesting.where can we find a benchmark which shows how this
pr improves the memory?
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/590
@lvfangmin In which case you need to add exceptions to
`config/findbugsExcludeFile.xml`.
---
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/590
@hanm the code complained by Findbug are all correct and expected:
1. System.exit if create watch manager failed in DataTree
It's using the factory to create the watch manager based
25 matches
Mail list logo
