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/06/03 01:03:16 UTC
[GitHub] curator pull request: Curator 110
GitHub user cammckenzie opened a pull request:
https://github.com/apache/curator/pull/9
Curator 110
Fix for CURATOR-110
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cammckenzie/curator CURATOR-110
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/9.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 #9
----
commit 1a63a102ebbaaead265babd71a3d52928f848bd0
Author: Cameron McKenzie <ca...@unico.com.au>
Date: 2014-06-02T06:30:26Z
CURATOR-110 - Modified state handling to treat 'CONNECTED' and
'RECONNECTED' events in the same way. Added a test case for the leader
latch being started before a connection to ZK has been established.
commit 0b7ae7e3672a9c817fe31144cd6b18d9a357f124
Author: Cameron McKenzie <ca...@unico.com.au>
Date: 2014-06-02T23:01:31Z
CURATOR-110 - Fixed up formatting to Curator standards
----
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13734197
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -162,6 +166,21 @@ public void process(WatchedEvent watchedEvent)
failedDeleteManager = new FailedDeleteManager(this);
namespaceFacadeCache = new NamespaceFacadeCache(this);
+
+ //Add callback handler to determine connection state transitions
+ getConnectionStateListenable().addListener(new ConnectionStateListener()
--- End diff --
OK - the listener might be needed, but not the atomic ref. That's available from ConnectionStateManager.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13730057
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -144,6 +148,14 @@ public LeaderLatch(CuratorFramework client, String latchPath, String id, CloseMo
this.id = Preconditions.checkNotNull(id, "id cannot be null");
this.closeMode = Preconditions.checkNotNull(closeMode, "closeMode cannot be null");
}
+
+ private CountDownLatch startLatch;
--- End diff --
Oops, was using this to try and reproduce the issue and forgot to remove it.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13364914
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java ---
@@ -57,5 +57,15 @@
* <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
* The connection will remain in read only mode until another state change is sent.
*/
- READ_ONLY
+ READ_ONLY;
+
+ /**
+ * Check if this state indicates that Curator has a connection to ZooKeeper
+ *
+ * @return True if connected, false otherwise
+ */
+ public boolean isConnected()
+ {
+ return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
--- End diff --
What's your definition of abstract in this context given that enum's can't
be abstract?
Do you just mean giving the enum a constructor with a boolean indicating
whether it represents a boolean state? And have an isConnected() and an
isDisconnected() method on the enum returning the state of this boolean
(and it's inverse)?
cheers
On Wed, Jun 4, 2014 at 9:16 AM, Jordan Zimmerman <no...@github.com>
wrote:
> In
> curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java:
>
> > @@ -57,5 +57,15 @@
> > * <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
> > * The connection will remain in read only mode until another state change is sent.
> > */
> > - READ_ONLY
> > + READ_ONLY;
> > +
> > + /**
> > + * Check if this state indicates that Curator has a connection to ZooKeeper
> > + *
> > + * @return True if connected, false otherwise
> > + */
> > + public boolean isConnected()
> > + {
> > + return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
>
> I'd prefer to see this as an abstract method and have each enum return the
> correct value
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/curator/pull/9/files#r13364526>.
>
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46078671
Ok, I've implemented it in the CuratorZookeeperClient. Doesn't seem to be any issues with using an arbitrary sleep length. Still need to do a bit of testing. Will get something sorted early next week hopefully.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46207798
The test are flakey because many of them have to wait "for a bit" for things to settle (session to fail, ephemeral to delete, etc.). But, the ZK server has essentially random behavior in terms of when it reconnects, connects, etc. If the VM is gc'ing or your system is slow, etc. it gets worse.
I keep trying to tune things but it's still not perfect.
---
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 110
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/9
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13364993
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java ---
@@ -57,5 +57,15 @@
* <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
* The connection will remain in read only mode until another state change is sent.
*/
- READ_ONLY
+ READ_ONLY;
+
+ /**
+ * Check if this state indicates that Curator has a connection to ZooKeeper
+ *
+ * @return True if connected, false otherwise
+ */
+ public boolean isConnected()
+ {
+ return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
--- End diff --
isConnected can be abstract:
```java
abstract public boolean isConnected();
```
Then each enum value will implement it. It's cleaner and puts the isConnected value close to the enum itself. Also, if we ever add new values it forces us to implement isConnected.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46041824
I'd appreciate your opinion. I'm OK with either.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13365128
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java ---
@@ -57,5 +57,15 @@
* <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
* The connection will remain in read only mode until another state change is sent.
*/
- READ_ONLY
+ READ_ONLY;
+
+ /**
+ * Check if this state indicates that Curator has a connection to ZooKeeper
+ *
+ * @return True if connected, false otherwise
+ */
+ public boolean isConnected()
+ {
+ return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
--- End diff --
There you go, you learn something new everyday! I knew that enums could
implement interfaces, and that they themselves couldn't be abstract but I
didn't realise you could still put abstract methods on them.
As far as an isDisconnected() method goes, you're happy with this just
being defined as !isConnected()?
On Wed, Jun 4, 2014 at 9:31 AM, Jordan Zimmerman <no...@github.com>
wrote:
> In
> curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java:
>
> > @@ -57,5 +57,15 @@
> > * <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
> > * The connection will remain in read only mode until another state change is sent.
> > */
> > - READ_ONLY
> > + READ_ONLY;
> > +
> > + /**
> > + * Check if this state indicates that Curator has a connection to ZooKeeper
> > + *
> > + * @return True if connected, false otherwise
> > + */
> > + public boolean isConnected()
> > + {
> > + return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
>
> isConnected can be abstract:
>
> abstract public boolean isConnected();
>
> Then each enum value will implement it. It's cleaner and puts the
> isConnected value close to the enum itself. Also, if we ever add new values
> it forces us to implement isConnected.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/curator/pull/9/files#r13364993>.
>
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699616
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -154,8 +166,30 @@ public void start() throws Exception
{
Preconditions.checkState(state.compareAndSet(State.LATENT, State.STARTED), "Cannot be started more than once");
- client.getConnectionStateListenable().addListener(listener);
- reset();
+ //Block until connected
+ final ExecutorService executor = ThreadUtils.newSingleThreadExecutor("");
+ executor.submit(new Runnable()
--- End diff --
This would be better as a utility ala RetryLoop.callWithRetry(). I can see this pattern getting used in other places.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13368056
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java ---
@@ -57,5 +57,15 @@
* <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
* The connection will remain in read only mode until another state change is sent.
*/
- READ_ONLY
+ READ_ONLY;
+
+ /**
+ * Check if this state indicates that Curator has a connection to ZooKeeper
+ *
+ * @return True if connected, false otherwise
+ */
+ public boolean isConnected()
+ {
+ return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
--- End diff --
I have updated the connection state to have an abstract isConnected()
method and added this to the pull request.
cheers
On Wed, Jun 4, 2014 at 9:37 AM, Jordan Zimmerman <no...@github.com>
wrote:
> In
> curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java:
>
> > @@ -57,5 +57,15 @@
> > * <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
> > * The connection will remain in read only mode until another state change is sent.
> > */
> > - READ_ONLY
> > + READ_ONLY;
> > +
> > + /**
> > + * Check if this state indicates that Curator has a connection to ZooKeeper
> > + *
> > + * @return True if connected, false otherwise
> > + */
> > + public boolean isConnected()
> > + {
> > + return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
>
> Actually, i think you're right. isLost() isn't necessary as it's implied
> as the opposite if isConnected().
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/curator/pull/9/files#r13365204>.
>
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13730278
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -162,6 +166,21 @@ public void process(WatchedEvent watchedEvent)
failedDeleteManager = new FailedDeleteManager(this);
namespaceFacadeCache = new NamespaceFacadeCache(this);
+
+ //Add callback handler to determine connection state transitions
+ getConnectionStateListenable().addListener(new ConnectionStateListener()
--- End diff --
We need to get updates on when the state changes, otherwise we'd need to keep polling the connectionStateManager for connection state changes. This is only needed if the blocking until connected logic is implemented in the CuratorFramework. If we use the CuratorZooKeeperClient instead, it can be 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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46244024
The ConnectionState.isConnected() is being used is in the CuratorFrameworkImpl when we're blocking for a connection. I think that leaving it in is probably not a bad idea, as it's still got potential to be useful in other places too.
In regards to the tests, I wonder if there's a way of structuring them to be more event driven rather than just waiting some arbitrary amount of time and hope an event occurs within that window? I'll have a look and see if I can find anything, but that's for another JIRA ticket.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699530
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -162,6 +166,21 @@ public void process(WatchedEvent watchedEvent)
failedDeleteManager = new FailedDeleteManager(this);
namespaceFacadeCache = new NamespaceFacadeCache(this);
+
+ //Add callback handler to determine connection state transitions
+ getConnectionStateListenable().addListener(new ConnectionStateListener()
--- End diff --
Why is this needed? It exists already in the connectionStateManager.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13730137
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -154,8 +166,30 @@ public void start() throws Exception
{
Preconditions.checkState(state.compareAndSet(State.LATENT, State.STARTED), "Cannot be started more than once");
- client.getConnectionStateListenable().addListener(listener);
- reset();
+ //Block until connected
+ final ExecutorService executor = ThreadUtils.newSingleThreadExecutor("");
+ executor.submit(new Runnable()
--- End diff --
Ok
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-45964727
OK - I see your point. I forget why the CuratorZookeeperClient does a spin loop like that. That's some of the oldest code in the lib.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-45963768
Man - it's getting complicated huh? I guess there's no other way. I wonder if there's some simplification that can be 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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13364526
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java ---
@@ -57,5 +57,15 @@
* <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
* The connection will remain in read only mode until another state change is sent.
*/
- READ_ONLY
+ READ_ONLY;
+
+ /**
+ * Check if this state indicates that Curator has a connection to ZooKeeper
+ *
+ * @return True if connected, false otherwise
+ */
+ public boolean isConnected()
+ {
+ return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
--- End diff --
I'd prefer to see this as an abstract method and have each enum return the correct value
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-45966251
Ok, so what's the way forward? We can either refactor internalBlockUntilConnectedOrTimedOut() in CuratorZooKeeperClient to allow arbitrary timeouts, and try to remove the 1 second sleep increments. Or, we can move the wait logic to the CuratorFramework, and use the ConnectionStateListener.
Assuming there's not technical reason why the CuratorZooKeeperClient can't block for arbitrary lengths of time, then it's ok to implement there. I don't really have a strong preference either way.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13730145
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -154,8 +166,30 @@ public void start() throws Exception
{
Preconditions.checkState(state.compareAndSet(State.LATENT, State.STARTED), "Cannot be started more than once");
- client.getConnectionStateListenable().addListener(listener);
- reset();
+ //Block until connected
+ final ExecutorService executor = ThreadUtils.newSingleThreadExecutor("");
--- End diff --
Ok, forgot to fill this in
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699509
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java ---
@@ -210,4 +213,28 @@
* @param watcher the watcher
*/
public void clearWatcherReferences(Watcher watcher);
+
+ /**
+ * Get the current connection state. The connection state will have a value of 0 until
+ * the first connection related event is received.
+ * @return The current connection state, or null if it is unknown
+ */
+ public ConnectionState getCurrentConnectionState();
+
+ /**
+ * Block until a connection to ZooKeeper is available or the maxWaitTime has been exceeded
+ * @param maxWaitTime The maximum wait time. Specify a value <= 0 to wait indefinitely
+ * @param units The time units for the maximum wait time.
+ * @return True if connection has been established, false otherwise.
+ * @throws InterruptedException If interrupted while waiting
+ */
+ public boolean blockUntilConnected(int maxWaitTime, TimeUnit units) throws InterruptedException;
--- End diff --
Why are these needed? They already exist in the CuratorZookeeperClient.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-45964598
For what I originally thought was going to be a 1 line fix, yes!
It seems to me that the wait logic is cleaner to implement at the CuratorFramework level, rather than at the CuratorZooKeeperClient, just because you've got a nice ConnectionStateListener framework to deal with. The internalBlockUntilConnectedOrTimedOut() method is a bit ugly in that it has to block in 1 second increments. Is there a reason that this can't just wait until for the entire session timeout in one go?
As an aside, it's a bit inefficient too as it allocates and destroys a CountDownLatch, and Watcher each iteration of the loop.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13730021
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java ---
@@ -210,4 +213,28 @@
* @param watcher the watcher
*/
public void clearWatcherReferences(Watcher watcher);
+
+ /**
+ * Get the current connection state. The connection state will have a value of 0 until
+ * the first connection related event is received.
+ * @return The current connection state, or null if it is unknown
+ */
+ public ConnectionState getCurrentConnectionState();
+
+ /**
+ * Block until a connection to ZooKeeper is available or the maxWaitTime has been exceeded
+ * @param maxWaitTime The maximum wait time. Specify a value <= 0 to wait indefinitely
+ * @param units The time units for the maximum wait time.
+ * @return True if connection has been established, false otherwise.
+ * @throws InterruptedException If interrupted while waiting
+ */
+ public boolean blockUntilConnected(int maxWaitTime, TimeUnit units) throws InterruptedException;
--- End diff --
The blockUntilConnectedOrTimedOut() method doesn't do what is needed in this case, because there's no way of specifying a timeout. We could call it in a loop I guess, but would need a second thread to interrupt it when our specified time out occurred, which is pretty ugly. Alternatively, we could modify the CuratorZookeeperClient to expose a blockUntilConnected() method that takes a time out? And have blockUntilConnectedOrTimedOut call this with the connection time out? Preferences?
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46147361
Latest commit has everything implemented at the CuratorFramework level. Have a look and see what you think. I still seem to have random completely unrelated tests failing occasionally which is a bit disconcerting. They work fine when I rerun them though. I'm not sure if these are due to race conditions or flakiness of the TestingServer.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13364513
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -542,34 +542,21 @@ public void processResult(CuratorFramework client, CuratorEvent event) throws Ex
private void handleStateChange(ConnectionState newState)
{
- switch ( newState )
- {
- default:
- {
- // NOP
- break;
- }
-
- case RECONNECTED:
+ if (newState.isConnected())
{
try
{
reset();
}
- catch ( Exception e )
+ catch (Exception e)
{
log.error("Could not reset leader latch", e);
setLeadership(false);
}
- break;
}
-
- case SUSPENDED:
- case LOST:
+ else
--- End diff --
Hmm - this suggests that we should have an isLost() method in the ConnectionState no?
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699515
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java ---
@@ -210,4 +213,28 @@
* @param watcher the watcher
*/
public void clearWatcherReferences(Watcher watcher);
+
+ /**
+ * Get the current connection state. The connection state will have a value of 0 until
+ * the first connection related event is received.
+ * @return The current connection state, or null if it is unknown
+ */
+ public ConnectionState getCurrentConnectionState();
+
+ /**
+ * Block until a connection to ZooKeeper is available or the maxWaitTime has been exceeded
+ * @param maxWaitTime The maximum wait time. Specify a value <= 0 to wait indefinitely
+ * @param units The time units for the maximum wait time.
+ * @return True if connection has been established, false otherwise.
+ * @throws InterruptedException If interrupted while waiting
+ */
+ public boolean blockUntilConnected(int maxWaitTime, TimeUnit units) throws InterruptedException;
+
+ /**
+ * Block until a connection to ZooKeeper is available. This method will not return until a
+ * connection is available or it is interrupted, in which case an InterruptedException will
+ * be thrown
+ * @throws InterruptedException If interrupted while waiting
+ */
+ public void blockUntilConnected() throws InterruptedException;
--- End diff --
Why are these needed? They already exist in the CuratorZookeeperClient.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46219971
Interestingly, now that we have ExecuteAfterConnectionEstablished, we no longer need ConnectionState.isConnected() nor do we need the change to LeaderLatch.handleStateChange().
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699571
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -144,6 +148,14 @@ public LeaderLatch(CuratorFramework client, String latchPath, String id, CloseMo
this.id = Preconditions.checkNotNull(id, "id cannot be null");
this.closeMode = Preconditions.checkNotNull(closeMode, "closeMode cannot be null");
}
+
+ private CountDownLatch startLatch;
--- End diff --
What's this for?
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13734170
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java ---
@@ -210,4 +213,28 @@
* @param watcher the watcher
*/
public void clearWatcherReferences(Watcher watcher);
+
+ /**
+ * Get the current connection state. The connection state will have a value of 0 until
+ * the first connection related event is received.
+ * @return The current connection state, or null if it is unknown
+ */
+ public ConnectionState getCurrentConnectionState();
+
+ /**
+ * Block until a connection to ZooKeeper is available or the maxWaitTime has been exceeded
+ * @param maxWaitTime The maximum wait time. Specify a value <= 0 to wait indefinitely
+ * @param units The time units for the maximum wait time.
+ * @return True if connection has been established, false otherwise.
+ * @throws InterruptedException If interrupted while waiting
+ */
+ public boolean blockUntilConnected(int maxWaitTime, TimeUnit units) throws InterruptedException;
--- End diff --
Technically, there is a timeout. It's the connectionTimeout used when creating the CuratorFramework instance. Is that not sufficient? If it's still needed, it seems better to push it down to CuratorZookeeperClient with the other method.
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/9#issuecomment-46139081
Scratch that, I think that the reason that the CuratorZookeeperClient was doing a spin loop was because there's a race condition with the watchers. It's possible for the local watcher to get a 'connected' event before the ConnectionState gets its 'connected' event. This means that when you call into ConnectionState.isConnected() it returns false, even though we know it's actually true.
So, while we could return the boolean based on what we know the state to be, it's going to be inconsistent for a short period with the ConnectionState, and this has potential for knock on consequences, even though the window of inconsistency is short.
So, I think it's actually better to move this wait logic into the CuratorFramework and use the ConnectionStateListener.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699582
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java ---
@@ -154,8 +166,30 @@ public void start() throws Exception
{
Preconditions.checkState(state.compareAndSet(State.LATENT, State.STARTED), "Cannot be started more than once");
- client.getConnectionStateListenable().addListener(listener);
- reset();
+ //Block until connected
+ final ExecutorService executor = ThreadUtils.newSingleThreadExecutor("");
--- End diff --
Pass in the name of the class instead of ""
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13365204
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionState.java ---
@@ -57,5 +57,15 @@
* <a href="http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode">http://wiki.apache.org/hadoop/ZooKeeper/GSoCReadOnlyMode</a>.
* The connection will remain in read only mode until another state change is sent.
*/
- READ_ONLY
+ READ_ONLY;
+
+ /**
+ * Check if this state indicates that Curator has a connection to ZooKeeper
+ *
+ * @return True if connected, false otherwise
+ */
+ public boolean isConnected()
+ {
+ return this == CONNECTED || this == RECONNECTED || this == READ_ONLY;
--- End diff --
Actually, i think you're right. isLost() isn't necessary as it's implied as the opposite if isConnected().
---
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 110
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13734392
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java ---
@@ -210,4 +213,28 @@
* @param watcher the watcher
*/
public void clearWatcherReferences(Watcher watcher);
+
+ /**
+ * Get the current connection state. The connection state will have a value of 0 until
+ * the first connection related event is received.
+ * @return The current connection state, or null if it is unknown
+ */
+ public ConnectionState getCurrentConnectionState();
+
+ /**
+ * Block until a connection to ZooKeeper is available or the maxWaitTime has been exceeded
+ * @param maxWaitTime The maximum wait time. Specify a value <= 0 to wait indefinitely
+ * @param units The time units for the maximum wait time.
+ * @return True if connection has been established, false otherwise.
+ * @throws InterruptedException If interrupted while waiting
+ */
+ public boolean blockUntilConnected(int maxWaitTime, TimeUnit units) throws InterruptedException;
--- End diff --
I just thought it's nicer to give control to the caller as to how long they want to wait for rather than being bound to the connection timeout. So, you would prefer if I exposed a blockUntilConnected() method on the CuratorZookeeperClient that takes a timeout, and then refactor the current blockUntilConnectedOrTimedOut to call that with the connection timeout? The only issue with this is that the current implementation of blockUntilConnectedOrTimedOut is implemented with 1 second sleeps, so the best granularity as far as timeouts you're going to get is a full second. This was partly why I implemented it via a connection state listener in the CuratorFramework. It just seemed cleaner than sitting in a loop sleeping for a second and then checking the state. I don't think that the connection state listeners are available from the CuratorZookeeperClient though.
---
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 110
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/9#discussion_r13699525
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -65,6 +67,7 @@
private final BlockingQueue<OperationAndData<?>> backgroundOperations;
private final NamespaceImpl namespace;
private final ConnectionStateManager connectionStateManager;
+ private final AtomicReference<ConnectionState> connectionState;
--- End diff --
Why is this needed? It exists already in the connectionStateManager.
---
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.
---