You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by Randgalt <gi...@git.apache.org> on 2015/08/28 20:41:28 UTC
[GitHub] curator pull request: [CURATOR-253] Use new injectSessionExpiratio...
GitHub user Randgalt opened a pull request:
https://github.com/apache/curator/pull/100
[CURATOR-253] Use new injectSessionExpiration() in the test module
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/curator CURATOR-253
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/100.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 #100
----
commit d1b4cbf070046ace5c047cc46d99c0ae71f749a5
Author: randgalt <ra...@apache.org>
Date: 2015-08-28T18:39:22Z
Switch to new injectSessionExpiration()
commit 145da217ff35df0178823da784f4dd1618851c5e
Author: randgalt <ra...@apache.org>
Date: 2015-08-28T18:39:54Z
New injectSessionExpiration() operates much faster than previously. It exposes an assumption in the tests. Added a debug hook to work around
----
---
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-253] Use new injectSessionExpiratio...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38373012
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentEphemeralNode.java ---
@@ -634,11 +640,6 @@ private CuratorFramework newCurator() throws IOException
return client;
}
- public void killSession(CuratorFramework curator) throws Exception
- {
- KillSession.kill(curator.getZookeeperClient().getZooKeeper(), curator.getZookeeperClient().getCurrentConnectionString());
--- End diff --
I was wondering this 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-253] Use new injectSessionExpiratio...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38372819
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java ---
@@ -377,7 +377,6 @@ public void testKilledSession() throws Exception
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
KillSession.kill(client.getZookeeperClient().getZooKeeper(), server.getConnectString());
- assertEvent(TreeCacheEvent.Type.CONNECTION_SUSPENDED);
--- End diff --
Why does the CONNECTION_SUSPENDED event no longer occur? Is this because of the new lifecycle stuff for session loss?
---
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-253] Use new injectSessionExpiratio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/100
---
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-253] Use new injectSessionExpiratio...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38231696
--- Diff: curator-test/src/main/java/org/apache/curator/test/KillSession.java ---
@@ -18,24 +18,12 @@
*/
package org.apache.curator.test;
-import org.apache.zookeeper.WatchedEvent;
-import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.ZooKeeper;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
/**
* <p>
- * Utility to simulate a ZK session dying. See: <a href="http://wiki.apache.org/hadoop/ZooKeeper/FAQ#A4">ZooKeeper FAQ</a>
+ * Utility to simulate a ZK session dying.
* </p>
- *
- * <blockquote>
- * In the case of testing we want to cause a problem, so to explicitly expire a session an
- * application connects to ZooKeeper, saves the session id and password, creates another
- * ZooKeeper handle with that id and password, and then closes the new handle. Since both
- * handles reference the same session, the close on second handle will invalidate the session
- * causing a SESSION_EXPIRED on the first handle.
- * </blockquote>
*/
public class KillSession
--- End diff --
Yeah, probably a good thing. I'll add 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-253] Use new injectSessionExpiratio...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38230977
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentEphemeralNode.java ---
@@ -111,11 +112,26 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
{
if ( newState == ConnectionState.RECONNECTED )
{
+ if ( debugReconnectLatch != null )
+ {
+ try
+ {
+ debugReconnectLatch.await();
+ }
+ catch ( InterruptedException e )
+ {
+ Thread.currentThread().interrupt();
+ e.printStackTrace();
+ }
+ }
createNode();
}
}
};
+ @VisibleForTesting
+ volatile CountDownLatch debugReconnectLatch = null;
--- End diff --
Why does this need to be volatile?
---
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-253] Use new injectSessionExpiratio...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38373083
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentEphemeralNode.java ---
@@ -634,11 +640,6 @@ private CuratorFramework newCurator() throws IOException
return client;
}
- public void killSession(CuratorFramework curator) throws Exception
- {
- KillSession.kill(curator.getZookeeperClient().getZooKeeper(), curator.getZookeeperClient().getCurrentConnectionString());
--- End diff --
Because no other code uses it. We should have a general solution.
---
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-253] Use new injectSessionExpiratio...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38234785
--- Diff: curator-test/src/main/java/org/apache/curator/test/KillSession.java ---
@@ -18,24 +18,12 @@
*/
package org.apache.curator.test;
-import org.apache.zookeeper.WatchedEvent;
-import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.ZooKeeper;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
/**
* <p>
- * Utility to simulate a ZK session dying. See: <a href="http://wiki.apache.org/hadoop/ZooKeeper/FAQ#A4">ZooKeeper FAQ</a>
+ * Utility to simulate a ZK session dying.
* </p>
- *
- * <blockquote>
- * In the case of testing we want to cause a problem, so to explicitly expire a session an
- * application connects to ZooKeeper, saves the session id and password, creates another
- * ZooKeeper handle with that id and password, and then closes the new handle. Since both
- * handles reference the same session, the close on second handle will invalidate the session
- * causing a SESSION_EXPIRED on the first handle.
- * </blockquote>
*/
public class KillSession
--- End diff --
Hmm - the only problem is where to put it. We don't currently build test Jars. So, I'd have to have a version for each module. That's ugly.
---
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-253] Use new injectSessionExpiratio...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38373821
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java ---
@@ -377,7 +377,6 @@ public void testKilledSession() throws Exception
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
KillSession.kill(client.getZookeeperClient().getZooKeeper(), server.getConnectString());
- assertEvent(TreeCacheEvent.Type.CONNECTION_SUSPENDED);
--- 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-253] Use new injectSessionExpiratio...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38231138
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentEphemeralNode.java ---
@@ -111,11 +112,26 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
{
if ( newState == ConnectionState.RECONNECTED )
{
+ if ( debugReconnectLatch != null )
+ {
+ try
+ {
+ debugReconnectLatch.await();
+ }
+ catch ( InterruptedException e )
+ {
+ Thread.currentThread().interrupt();
+ e.printStackTrace();
+ }
+ }
createNode();
}
}
};
+ @VisibleForTesting
+ volatile CountDownLatch debugReconnectLatch = null;
--- End diff --
Can't predict what thread it will come from. In the current use case it doesn't need to be, but who knows in the future.
---
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-253] Use new injectSessionExpiratio...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38373795
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentEphemeralNode.java ---
@@ -634,11 +640,6 @@ private CuratorFramework newCurator() throws IOException
return client;
}
- public void killSession(CuratorFramework curator) throws Exception
- {
- KillSession.kill(curator.getZookeeperClient().getZooKeeper(), curator.getZookeeperClient().getCurrentConnectionString());
--- End diff --
Maybe it should go into the BaseClassForTests?
---
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-253] Use new injectSessionExpiratio...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38231122
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentEphemeralNode.java ---
@@ -634,11 +640,6 @@ private CuratorFramework newCurator() throws IOException
return client;
}
- public void killSession(CuratorFramework curator) throws Exception
- {
- KillSession.kill(curator.getZookeeperClient().getZooKeeper(), curator.getZookeeperClient().getCurrentConnectionString());
--- End diff --
Why not leave the method and have it do the new thing instead of repeating the same snippet in a bunch of 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-253] Use new injectSessionExpiratio...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38231256
--- Diff: curator-test/src/main/java/org/apache/curator/test/KillSession.java ---
@@ -18,24 +18,12 @@
*/
package org.apache.curator.test;
-import org.apache.zookeeper.WatchedEvent;
-import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.ZooKeeper;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
/**
* <p>
- * Utility to simulate a ZK session dying. See: <a href="http://wiki.apache.org/hadoop/ZooKeeper/FAQ#A4">ZooKeeper FAQ</a>
+ * Utility to simulate a ZK session dying.
* </p>
- *
- * <blockquote>
- * In the case of testing we want to cause a problem, so to explicitly expire a session an
- * application connects to ZooKeeper, saves the session id and password, creates another
- * ZooKeeper handle with that id and password, and then closes the new handle. Since both
- * handles reference the same session, the close on second handle will invalidate the session
- * causing a SESSION_EXPIRED on the first handle.
- * </blockquote>
*/
public class KillSession
--- End diff --
New method suggestion: KillSession::kill(CuratorFramework) and that can extract the zookeeper bits on it's own. More robust for future improvement, maybe?
---
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-253] Use new injectSessionExpiratio...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/100#discussion_r38372964
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java ---
@@ -377,7 +377,6 @@ public void testKilledSession() throws Exception
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
KillSession.kill(client.getZookeeperClient().getZooKeeper(), server.getConnectString());
- assertEvent(TreeCacheEvent.Type.CONNECTION_SUSPENDED);
--- End diff --
Killing the session goes immediately to LOST. It actually was always a possibility but now it does it so fast that it's always the case.
---
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.
---