You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by ramaraochavali <gi...@git.apache.org> on 2018/08/20 06:49:20 UTC
[GitHub] curator pull request #278: [CURATOR-477] added support for turning off zk wa...
GitHub user ramaraochavali opened a pull request:
https://github.com/apache/curator/pull/278
[CURATOR-477] added support for turning off zk watches
In our use case, we use `TreeCache` to get Zk Data periodically. We start `TreeCache` read data and close it. In this use case, The `ZkWatchManager` of `ZooKeeper` class keeps growing for every TreeCache operation because new `TreeNode` objects are created and added there leading to a memory leak. Also since we do not want the Watcher to periodically watch, this creates unnecessary background operations.
In this PR, Made the createZkWatches configurable in the Builder, so that use cases like ours can set it to false. The default is true, retaining the current behaviour.
Fixes the jira issue https://issues.apache.org/jira/browse/CURATOR-477
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ramaraochavali/curator CURATOR-477
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/278.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 #278
----
commit d4a0d95488345bfb0983553bfbfbc40643ac031b
Author: Rama <ra...@...>
Date: 2018-08-20T06:46:12Z
added support for zkwatches
----
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by ramaraochavali <gi...@git.apache.org>.
Github user ramaraochavali commented on the issue:
https://github.com/apache/curator/pull/278
@dragonsinth sorry. Added one more commit for exists check also. can you PTAL?
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by ramaraochavali <gi...@git.apache.org>.
Github user ramaraochavali commented on the issue:
https://github.com/apache/curator/pull/278
@dragonsinth Thanks for your review. addressed the feedback. PTAL
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/278
aha that would do it! I do think you should update the `maybeWatch` generic method per my other comment
---
[GitHub] curator pull request #278: [CURATOR-477] added support for turning off zk wa...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/278#discussion_r219688410
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -263,11 +281,20 @@ private void doRefreshData() throws Exception
{
if ( dataIsCompressed )
{
- client.getData().decompressed().usingWatcher(this).inBackground(this).forPath(path);
+ if (createZkWatches) {
+ client.getData().decompressed().usingWatcher(this).inBackground(this).forPath(path);
+ } else {
+ client.getData().decompressed().inBackground(this).forPath(path);
+ }
--- End diff --
feel like this could all be a little cleaner with a maybeWatch() helper in the class, e.g.
```
maybeWatch(client.getData().decompressed()).inBackground(this).forPath(path)
```
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by ramaraochavali <gi...@git.apache.org>.
Github user ramaraochavali commented on the issue:
https://github.com/apache/curator/pull/278
Ahh. Sorry. I missed that comment. I have changed it now. PTAL.
---
[GitHub] curator pull request #278: [CURATOR-477] added support for turning off zk wa...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/278#discussion_r219703563
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -263,15 +284,31 @@ private void doRefreshData() throws Exception
{
if ( dataIsCompressed )
{
- client.getData().decompressed().usingWatcher(this).inBackground(this).forPath(path);
+ maybeWatch(client.getData().decompressed()).forPath(path);
}
else
{
- client.getData().usingWatcher(this).inBackground(this).forPath(path);
+ maybeWatch(client.getData()).forPath(path);
}
}
}
+ private ErrorListenerPathable<byte[]> maybeWatch(GetDataWatchBackgroundStatable dataBuilder) {
+ if (disableZkWatches) {
+ return dataBuilder.inBackground(this);
+ } else {
+ return dataBuilder.usingWatcher(this).inBackground(this);
+ }
+ }
+
+ private ErrorListenerPathable<byte[]> maybeWatch(GetDataBuilder dataBuilder) {
+ if (disableZkWatches) {
+ return dataBuilder.inBackground(this);
+ } else {
+ return dataBuilder.usingWatcher(this).inBackground(this);
+ }
+ }
+
--- End diff --
```java
private void doRefreshChildren() throws Exception
{
if ( treeState.get() == TreeState.STARTED )
{
maybeWatch(client.getChildren()).forPath(path);
}
}
private void doRefreshData() throws Exception
{
if ( treeState.get() == TreeState.STARTED )
{
if ( dataIsCompressed )
{
maybeWatch(client.getData().decompressed()).forPath(path);
}
else
{
maybeWatch(client.getData()).forPath(path);
}
}
}
private <T, P extends Watchable<BackgroundPathable<T>> & BackgroundPathable<T>> Pathable<T> maybeWatch(P dataBuilder)
{
if ( disableZkWatches )
{
return dataBuilder.inBackground(this);
}
else
{
return dataBuilder.usingWatcher(this).inBackground(this);
}
}
```
---
[GitHub] curator pull request #278: [CURATOR-477] added support for turning off zk wa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/278
---
[GitHub] curator pull request #278: [CURATOR-477] added support for turning off zk wa...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/278#discussion_r219688441
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -165,6 +167,18 @@ public Builder setCreateParentNodes(boolean createParentNodes)
return this;
}
+ /**
+ * By default, TreeCache creates {@link org.apache.zookeeper.ZooKeeper} watches for every created path.
+ * Change this behavior with this method.
+ * @param createZkWatches true to create watches
+ * @return this for chaining
+ */
+ public Builder setCreateZkWatches(boolean createZkWatches)
+ {
+ this.createZkWatches = createZkWatches;
+ return this;
+ }
--- End diff --
and e.g. disableZkWatches(boolean)
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/278
TestTreeCache is getting stuck on your branch. Haven't figured out why yet. Wanna take a look?
---
[GitHub] curator pull request #278: [CURATOR-477] added support for turning off zk wa...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/278#discussion_r219688434
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -84,6 +85,7 @@
private ExecutorService executorService = null;
private int maxDepth = Integer.MAX_VALUE;
private boolean createParentNodes = false;
+ private boolean createZkWatches = true;
--- End diff --
I'd probably reverse this as "disableZkWatch" since that's the unusual case.
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/278
I will merge this shortly, thanks @ramaraochavali and @dragonsinth
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/278
@Randgalt LGTM, although I haven't rerun tests yet.
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by ramaraochavali <gi...@git.apache.org>.
Github user ramaraochavali commented on the issue:
https://github.com/apache/curator/pull/278
@Randgalt just following-up on this - Any thing else pending on this?
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Posted by ramaraochavali <gi...@git.apache.org>.
Github user ramaraochavali commented on the issue:
https://github.com/apache/curator/pull/278
Sorry. My bad. Problem with my change. Fixed. PTAL
---