You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by cammckenzie <gi...@git.apache.org> on 2014/12/01 23:23:54 UTC
[GitHub] curator pull request: Curator 161
GitHub user cammckenzie opened a pull request:
https://github.com/apache/curator/pull/56
Curator 161
First cut of watcher removal. Still need to provide the ability to register for notification of watch removal.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/curator CURATOR-161
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/56.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 #56
----
commit 034e910a907fa89e1c0cd3373fa3bf8ee7759ae6
Author: Ioannis Canellos <io...@gmail.com>
Date: 2014-11-06T14:53:09Z
Upgrade to ZooKeeper 3.5.0-alpha.
commit d4883a845a3778bcad4a776bb18b146f733d8885
Author: Ioannis Canellos <io...@gmail.com>
Date: 2014-11-06T15:30:29Z
Update project version to 3.0.0-SNAPSHOT.
commit 9ff9ccd23c8d033b2e7d72b83a0183d05f5dd685
Author: Cameron McKenzie <ca...@unico.com.au>
Date: 2014-12-01T22:16:40Z
CURATOR-161 - Initial cut of remove watches functionality. This provides
the ability to remove watches, but does not yet provide a framework for
observers being notified when a watch has been removed.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665989
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/ChaosMonkeyCnxnFactory.java ---
@@ -59,13 +59,6 @@ public void startup(ZooKeeperServer zks) throws IOException, InterruptedExceptio
super.startup(new ChaosMonkeyZookeeperServer(zks));
}
- /**
--- End diff --
Not sure? I don't see this change in my revision history? Last update was 6 months ago in the upgrade to ZK 3.5.0-alpha by Ioannis?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/56#issuecomment-103725842
Ok, thanks will merge into the 3.0.0 branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30012217
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesBuilder.java ---
@@ -0,0 +1,29 @@
+package org.apache.curator.framework.api;
--- End diff --
Maybe I mis-remembered. If the rest of the code doesn't check you don't need to.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30012206
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesBuilder.java ---
@@ -0,0 +1,29 @@
+package org.apache.curator.framework.api;
--- End diff --
Looking at the local case again, I think it's actually just client.getZookeeper() that needs to be used. I notice that this is how it's used in all of the performBackgroundOperation calls, but there are no checks for null pointers in this case. Is there a reason for this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30012227
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+import org.apache.curator.RetryLoop;
+import org.apache.curator.TimeTrace;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.BackgroundPathable;
+import org.apache.curator.framework.api.BackgroundPathableQuietly;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.framework.api.CuratorEventType;
+import org.apache.curator.framework.api.CuratorWatcher;
+import org.apache.curator.framework.api.Pathable;
+import org.apache.curator.framework.api.RemoveWatchesLocal;
+import org.apache.curator.framework.api.RemoveWatchesBuilder;
+import org.apache.curator.framework.api.RemoveWatchesType;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.Watcher.WatcherType;
+import org.apache.zookeeper.ZooKeeper;
+
+
+public class RemoveWatchesBuilderImpl implements RemoveWatchesBuilder, RemoveWatchesType, RemoveWatchesLocal, BackgroundOperation<String>
+{
+ private CuratorFrameworkImpl client;
+ private Watcher watcher;
+ private WatcherType watcherType;
+ private boolean local;
+ private boolean quietly;
+ private Backgrounding backgrounding;
+
+ public RemoveWatchesBuilderImpl(CuratorFrameworkImpl client)
+ {
+ this.client = client;
+ this.watcher = null;
+ this.watcherType = WatcherType.Any;
+ this.local = false;
+ this.quietly = false;
+ this.backgrounding = new Backgrounding();
+ }
+
+ @Override
+ public RemoveWatchesType remove(Watcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesType remove(CuratorWatcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesType removeAll()
+ {
+ this.watcher = null;
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesLocal ofType(WatcherType watcherType)
+ {
+ this.watcherType = watcherType;
+
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Object context)
+ {
+ backgrounding = new Backgrounding(callback, context);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Object context, Executor executor)
+ {
+ backgrounding = new Backgrounding(client, callback, context, executor);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback)
+ {
+ backgrounding = new Backgrounding(callback);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Executor executor)
+ {
+ backgrounding = new Backgrounding(client, callback, executor);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground()
+ {
+ backgrounding = new Backgrounding(true);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(Object context)
+ {
+ backgrounding = new Backgrounding(context);
+ return this;
+ }
+
+ @Override
+ public BackgroundPathableQuietly<Void> locally()
+ {
+ local = true;
+ return this;
+ }
+
+ @Override
+ public BackgroundPathable<Void> quietly()
+ {
+ quietly = true;
+ return this;
+ }
+
+ @Override
+ public Void forPath(String path) throws Exception
+ {
+ final String adjustedPath = client.fixForNamespace(path);
+
+ if(backgrounding.inBackground())
+ {
+ pathInBackground(adjustedPath);
+ }
+ else
+ {
+ pathInForeground(adjustedPath);
+ }
+
+ return null;
+ }
+
+ private void pathInBackground(String path)
+ {
+ OperationAndData.ErrorCallback<String> errorCallback = null;
+ client.processBackgroundOperation(new OperationAndData<String>(this, path, backgrounding.getCallback(), errorCallback, backgrounding.getContext()), null);
+ }
+
+ private void pathInForeground(final String path) throws Exception
+ {
+ RetryLoop.callWithRetry(client.getZookeeperClient(),
+ new Callable<Void>()
+ {
+ @Override
+ public Void call() throws Exception
+ {
+ try
+ {
+ ZooKeeper zkClient = client.getZooKeeper();
+ if(watcher == null)
--- End diff --
Don't forget to look at these lines. It seems wrong to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30011129
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesLocal.java ---
@@ -0,0 +1,17 @@
+package org.apache.curator.framework.api;
+
+/**
+ * Builder to allow the specification of whether it is acceptable to remove client side watch information
+ * in the case where ZK cannot be contacted.
+ */
+public interface RemoveWatchesLocal extends BackgroundPathableQuietly<Void>
+{
+
+ /**
+ * Specify if the client should just remove client side watches if a connection to ZK
+ * is not available.
+ * @return
+ */
+ public BackgroundPathableQuietly<Void> local();
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665596
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/BackgroundPathableQuietly.java ---
@@ -0,0 +1,5 @@
+package org.apache.curator.framework.api;
+
+public interface BackgroundPathableQuietly<T> extends BackgroundPathable<T>, Quietly<BackgroundPathable<T>>
--- End diff --
Naming convention nit: should be "BackgroundPathQuietlyable" - Yes, I'm probably going insane.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30099128
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesBuilder.java ---
@@ -0,0 +1,29 @@
+package org.apache.curator.framework.api;
--- End diff --
Internally, it looks like "local" mode acts only if the connection is lost. Maybe you can test the connection state when local is set and don't do retries, etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30088622
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+import org.apache.curator.RetryLoop;
+import org.apache.curator.TimeTrace;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.BackgroundPathable;
+import org.apache.curator.framework.api.BackgroundPathableQuietly;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.framework.api.CuratorEventType;
+import org.apache.curator.framework.api.CuratorWatcher;
+import org.apache.curator.framework.api.Pathable;
+import org.apache.curator.framework.api.RemoveWatchesLocal;
+import org.apache.curator.framework.api.RemoveWatchesBuilder;
+import org.apache.curator.framework.api.RemoveWatchesType;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.Watcher.WatcherType;
+import org.apache.zookeeper.ZooKeeper;
+
+
+public class RemoveWatchesBuilderImpl implements RemoveWatchesBuilder, RemoveWatchesType, RemoveWatchesLocal, BackgroundOperation<String>
+{
+ private CuratorFrameworkImpl client;
+ private Watcher watcher;
+ private WatcherType watcherType;
+ private boolean local;
+ private boolean quietly;
+ private Backgrounding backgrounding;
+
+ public RemoveWatchesBuilderImpl(CuratorFrameworkImpl client)
+ {
+ this.client = client;
+ this.watcher = null;
+ this.watcherType = WatcherType.Any;
+ this.local = false;
+ this.quietly = false;
+ this.backgrounding = new Backgrounding();
+ }
+
+ @Override
+ public RemoveWatchesType remove(Watcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesType remove(CuratorWatcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesType removeAll()
+ {
+ this.watcher = null;
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesLocal ofType(WatcherType watcherType)
+ {
+ this.watcherType = watcherType;
+
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Object context)
+ {
+ backgrounding = new Backgrounding(callback, context);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Object context, Executor executor)
+ {
+ backgrounding = new Backgrounding(client, callback, context, executor);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback)
+ {
+ backgrounding = new Backgrounding(callback);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Executor executor)
+ {
+ backgrounding = new Backgrounding(client, callback, executor);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground()
+ {
+ backgrounding = new Backgrounding(true);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(Object context)
+ {
+ backgrounding = new Backgrounding(context);
+ return this;
+ }
+
+ @Override
+ public BackgroundPathableQuietly<Void> locally()
+ {
+ local = true;
+ return this;
+ }
+
+ @Override
+ public BackgroundPathable<Void> quietly()
+ {
+ quietly = true;
+ return this;
+ }
+
+ @Override
+ public Void forPath(String path) throws Exception
+ {
+ final String adjustedPath = client.fixForNamespace(path);
+
+ if(backgrounding.inBackground())
+ {
+ pathInBackground(adjustedPath);
+ }
+ else
+ {
+ pathInForeground(adjustedPath);
+ }
+
+ return null;
+ }
+
+ private void pathInBackground(String path)
+ {
+ OperationAndData.ErrorCallback<String> errorCallback = null;
+ client.processBackgroundOperation(new OperationAndData<String>(this, path, backgrounding.getCallback(), errorCallback, backgrounding.getContext()), null);
+ }
+
+ private void pathInForeground(final String path) throws Exception
+ {
+ RetryLoop.callWithRetry(client.getZookeeperClient(),
+ new Callable<Void>()
+ {
+ @Override
+ public Void call() throws Exception
+ {
+ try
+ {
+ ZooKeeper zkClient = client.getZooKeeper();
+ if(watcher == null)
--- End diff --
What is it that seems wrong? If no Watcher is specified this means that we're removing all watches for a given path. If a Watcher is specified then this means we're removing a specific watch from a given path.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30666074
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/DeleteBuilder.java ---
@@ -18,6 +18,6 @@
*/
package org.apache.curator.framework.api;
-public interface DeleteBuilder extends Guaranteeable, ChildrenDeletable
+public interface DeleteBuilder extends GuaranteeableDelete, ChildrenDeletable
--- End diff --
Ok, changed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665705
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/FailedDeleteManager.java ---
@@ -19,45 +19,18 @@
package org.apache.curator.framework.imps;
import org.apache.curator.framework.CuratorFramework;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-class FailedDeleteManager
+class FailedDeleteManager extends FailedOperationManager<String>
{
- private final Logger log = LoggerFactory.getLogger(getClass());
- private final CuratorFramework client;
-
- volatile FailedDeleteManagerListener debugListener = null;
--- End diff --
nit: mark @VisibleForTesting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30666087
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/BackgroundPathableQuietly.java ---
@@ -0,0 +1,5 @@
+package org.apache.curator.framework.api;
+
+public interface BackgroundPathableQuietly<T> extends BackgroundPathable<T>, Quietly<BackgroundPathable<T>>
--- End diff --
Changed. I have never written Quitelyable before!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/56#issuecomment-101846294
All functionality is now implemented. Ready for a final review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665846
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/ChaosMonkeyCnxnFactory.java ---
@@ -59,13 +59,6 @@ public void startup(ZooKeeperServer zks) throws IOException, InterruptedExceptio
super.startup(new ChaosMonkeyZookeeperServer(zks));
}
- /**
--- End diff --
Why is this removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665521
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/DeleteBuilder.java ---
@@ -18,6 +18,6 @@
*/
package org.apache.curator.framework.api;
-public interface DeleteBuilder extends Guaranteeable, ChildrenDeletable
+public interface DeleteBuilder extends GuaranteeableDelete, ChildrenDeletable
--- End diff --
A nit, but the naming convention is actually: <Verb1>..<VerbN>able. So it would be GuaranteeDeletable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665756
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/FailedRemoveWatchManager.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.imps;
+
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.zookeeper.Watcher;
+
+class FailedRemoveWatchManager extends FailedOperationManager<FailedRemoveWatchManager.FailedRemoveWatchDetails>
+{
+ FailedRemoveWatchManager(CuratorFramework client)
+ {
+ super(client);
+ }
+
+ @Override
+ protected void executeGuaranteedOperationInBackground(FailedRemoveWatchDetails details)
+ throws Exception
+ {
+ if(details.watcher == null)
--- End diff --
nit: formatting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30099169
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+import org.apache.curator.RetryLoop;
+import org.apache.curator.TimeTrace;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.BackgroundPathable;
+import org.apache.curator.framework.api.BackgroundPathableQuietly;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.framework.api.CuratorEventType;
+import org.apache.curator.framework.api.CuratorWatcher;
+import org.apache.curator.framework.api.Pathable;
+import org.apache.curator.framework.api.RemoveWatchesLocal;
+import org.apache.curator.framework.api.RemoveWatchesBuilder;
+import org.apache.curator.framework.api.RemoveWatchesType;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.Watcher.WatcherType;
+import org.apache.zookeeper.ZooKeeper;
+
+
+public class RemoveWatchesBuilderImpl implements RemoveWatchesBuilder, RemoveWatchesType, RemoveWatchesLocal, BackgroundOperation<String>
+{
+ private CuratorFrameworkImpl client;
+ private Watcher watcher;
+ private WatcherType watcherType;
+ private boolean local;
+ private boolean quietly;
+ private Backgrounding backgrounding;
+
+ public RemoveWatchesBuilderImpl(CuratorFrameworkImpl client)
+ {
+ this.client = client;
+ this.watcher = null;
+ this.watcherType = WatcherType.Any;
+ this.local = false;
+ this.quietly = false;
+ this.backgrounding = new Backgrounding();
+ }
+
+ @Override
+ public RemoveWatchesType remove(Watcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesType remove(CuratorWatcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesType removeAll()
+ {
+ this.watcher = null;
+ return this;
+ }
+
+ @Override
+ public RemoveWatchesLocal ofType(WatcherType watcherType)
+ {
+ this.watcherType = watcherType;
+
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Object context)
+ {
+ backgrounding = new Backgrounding(callback, context);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Object context, Executor executor)
+ {
+ backgrounding = new Backgrounding(client, callback, context, executor);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback)
+ {
+ backgrounding = new Backgrounding(callback);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(BackgroundCallback callback, Executor executor)
+ {
+ backgrounding = new Backgrounding(client, callback, executor);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground()
+ {
+ backgrounding = new Backgrounding(true);
+ return this;
+ }
+
+ @Override
+ public Pathable<Void> inBackground(Object context)
+ {
+ backgrounding = new Backgrounding(context);
+ return this;
+ }
+
+ @Override
+ public BackgroundPathableQuietly<Void> locally()
+ {
+ local = true;
+ return this;
+ }
+
+ @Override
+ public BackgroundPathable<Void> quietly()
+ {
+ quietly = true;
+ return this;
+ }
+
+ @Override
+ public Void forPath(String path) throws Exception
+ {
+ final String adjustedPath = client.fixForNamespace(path);
+
+ if(backgrounding.inBackground())
+ {
+ pathInBackground(adjustedPath);
+ }
+ else
+ {
+ pathInForeground(adjustedPath);
+ }
+
+ return null;
+ }
+
+ private void pathInBackground(String path)
+ {
+ OperationAndData.ErrorCallback<String> errorCallback = null;
+ client.processBackgroundOperation(new OperationAndData<String>(this, path, backgrounding.getCallback(), errorCallback, backgrounding.getContext()), null);
+ }
+
+ private void pathInForeground(final String path) throws Exception
+ {
+ RetryLoop.callWithRetry(client.getZookeeperClient(),
+ new Callable<Void>()
+ {
+ @Override
+ public Void call() throws Exception
+ {
+ try
+ {
+ ZooKeeper zkClient = client.getZooKeeper();
+ if(watcher == null)
--- End diff --
I was wrong. Ignore this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30012701
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesBuilder.java ---
@@ -0,0 +1,29 @@
+package org.apache.curator.framework.api;
--- End diff --
There are additional problems when trying the local case in the background as the 'performBackgroundOperation()' method checks if the client is connected prior to executing the operation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30666113
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/FailedRemoveWatchManager.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.imps;
+
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.zookeeper.Watcher;
+
+class FailedRemoveWatchManager extends FailedOperationManager<FailedRemoveWatchManager.FailedRemoveWatchDetails>
+{
+ FailedRemoveWatchManager(CuratorFramework client)
+ {
+ super(client);
+ }
+
+ @Override
+ protected void executeGuaranteedOperationInBackground(FailedRemoveWatchDetails details)
+ throws Exception
+ {
+ if(details.watcher == null)
--- End diff --
Removed the extra space.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30088312
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
--- End diff --
I don't think that this is necessarily true is it? We don't want to remove the Watcher reference itself because it may still be watching other paths. If it is no longer referenced then the WeakReference in the NamespaceWatcherMaps internal cache should handle this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30011302
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesType.java ---
@@ -0,0 +1,19 @@
+package org.apache.curator.framework.api;
+
+import org.apache.zookeeper.Watcher.WatcherType;
+
+/**
+ * Builder to allow the specification of whether it is acceptable to remove client side watch information
+ * in the case where ZK cannot be contacted.
+ */
+public interface RemoveWatchesType
+{
+
+ /**
+ * Specify the type of watcher to be removed.
+ * @param watcherType
+ * @return
+ */
+ public RemoveWatchesLocal ofType(WatcherType watcherType);
--- End diff --
Done, it will now accept:
client.watches().removeAll().forPath("/");
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #56: Curator 161
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/56
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30086182
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+import org.apache.curator.RetryLoop;
+import org.apache.curator.TimeTrace;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.BackgroundPathable;
+import org.apache.curator.framework.api.BackgroundPathableQuietly;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.framework.api.CuratorEventType;
+import org.apache.curator.framework.api.CuratorWatcher;
+import org.apache.curator.framework.api.Pathable;
+import org.apache.curator.framework.api.RemoveWatchesLocal;
+import org.apache.curator.framework.api.RemoveWatchesBuilder;
+import org.apache.curator.framework.api.RemoveWatchesType;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.Watcher.WatcherType;
+import org.apache.zookeeper.ZooKeeper;
+
+
+public class RemoveWatchesBuilderImpl implements RemoveWatchesBuilder, RemoveWatchesType, RemoveWatchesLocal, BackgroundOperation<String>
+{
+ private CuratorFrameworkImpl client;
+ private Watcher watcher;
+ private WatcherType watcherType;
+ private boolean local;
+ private boolean quietly;
+ private Backgrounding backgrounding;
+
+ public RemoveWatchesBuilderImpl(CuratorFrameworkImpl client)
+ {
+ this.client = client;
+ this.watcher = null;
+ this.watcherType = null;
+ this.local = false;
+ this.quietly = false;
+ this.backgrounding = new Backgrounding();
+ }
+
+ @Override
+ public RemoveWatchesType remove(Watcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
--- End diff --
I guess if we don't have a reference in the NamespaceWatcherMap then we pass the watcher through to the ZK client without it being wrapper in a NamespaceWatcher. The only reason I can think of a watcher not being in the map would be if it was registered directly with the ZK client without going through the ZK framework. Either way, if it doesn't exist it will result in a NoWatcherException
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30067034
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+import org.apache.curator.RetryLoop;
+import org.apache.curator.TimeTrace;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.BackgroundPathable;
+import org.apache.curator.framework.api.BackgroundPathableQuietly;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.framework.api.CuratorEventType;
+import org.apache.curator.framework.api.CuratorWatcher;
+import org.apache.curator.framework.api.Pathable;
+import org.apache.curator.framework.api.RemoveWatchesLocal;
+import org.apache.curator.framework.api.RemoveWatchesBuilder;
+import org.apache.curator.framework.api.RemoveWatchesType;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.Watcher.WatcherType;
+import org.apache.zookeeper.ZooKeeper;
+
+
+public class RemoveWatchesBuilderImpl implements RemoveWatchesBuilder, RemoveWatchesType, RemoveWatchesLocal, BackgroundOperation<String>
+{
+ private CuratorFrameworkImpl client;
+ private Watcher watcher;
+ private WatcherType watcherType;
+ private boolean local;
+ private boolean quietly;
+ private Backgrounding backgrounding;
+
+ public RemoveWatchesBuilderImpl(CuratorFrameworkImpl client)
+ {
+ this.client = client;
+ this.watcher = null;
+ this.watcherType = null;
+ this.local = false;
+ this.quietly = false;
+ this.backgrounding = new Backgrounding();
+ }
+
+ @Override
+ public RemoveWatchesType remove(Watcher watcher)
+ {
+ this.watcher = watcher == null ? null : client.getNamespaceWatcherMap().getNamespaceWatcher(watcher);
--- End diff --
<code> client.getNamespaceWatcherMap().getNamespaceWatcher(watcher)</code> will add a namespace watcher record if it doesn't exist. That doesn't seem right for removal. A new method in NamespaceWatcherMap is probably needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30099181
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/RemoveWatchesBuilder.java ---
@@ -0,0 +1,29 @@
+package org.apache.curator.framework.api;
--- End diff --
Looking at the ZK code I think that 'local' mode occurs whenever there's any sort of error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665909
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcherMap.java ---
@@ -70,6 +70,14 @@ NamespaceWatcher remove(Object key)
{
return map.remove(key);
}
+
+ /**
+ * Remove all watchers for a given path
+ * @param path
+ */
+ void removeAllForPath(String path) {
+
--- End diff --
Looks like I started doing something with the NamespaceWatcherMap and then realised that I shouldn't. Will remove this method, it's not used anywhere.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #56: Curator 161
Posted by aaylward <gi...@git.apache.org>.
Github user aaylward commented on the issue:
https://github.com/apache/curator/pull/56
can this be closed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30061592
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
--- End diff --
Once the builder is committed via forPath() the watcher should also be removed from NamespaceWatcherMap. We should deprecate clearWatcherReferences() too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665781
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceWatcherMap.java ---
@@ -70,6 +70,14 @@ NamespaceWatcher remove(Object key)
{
return map.remove(key);
}
+
+ /**
+ * Remove all watchers for a given path
+ * @param path
+ */
+ void removeAllForPath(String path) {
+
--- End diff --
Why is this empty?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30100408
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/RemoveWatchesBuilderImpl.java ---
@@ -0,0 +1,214 @@
+package org.apache.curator.framework.imps;
--- End diff --
Good point. You're right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/56#issuecomment-103723252
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 161
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/56#discussion_r30665650
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -807,7 +821,7 @@ private void performBackgroundOperation(OperationAndData<?> operationAndData)
{
try
{
- if ( client.isConnected() )
+ if ( !operationAndData.isConnectionRequired() || client.isConnected() )
--- End diff --
Why this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---