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/17 05:51:24 UTC
[GitHub] curator pull request: CURATOR-114 - Modified the TestingServer to ...
GitHub user cammckenzie opened a pull request:
https://github.com/apache/curator/pull/11
CURATOR-114 - Modified the TestingServer to expose the restart() method
Modified the TestingServer to expose the restart() method on the underlying TestingZooKeeperServer. Modified all unit tests that were previously using the stop() and then recreate using existing temporary directory and port approach for restarting the server, so that they now just call the restart() method.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cammckenzie/curator CURATOR-114
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/11.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 #11
----
commit 27c70aab9a3d86ba9a8a20e3ae28ef5bed147300
Author: Cameron McKenzie <ca...@unico.com.au>
Date: 2014-06-17T03:49:03Z
CURATOR-114 - Modified the TestingServer to expose the restart() method
on the underlying TestingZooKeeperServer. Modified all unit tests that
were previously using the stop() and then recreate using existing
temporary directory and port approach for restarting the server, so that
they now just call the restart() 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-114 - Modified the TestingServer to ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/11#discussion_r13887642
--- Diff: curator-test/src/main/java/org/apache/curator/test/TestingServer.java ---
@@ -104,6 +171,17 @@ public void stop() throws IOException
}
/**
+ * Restart the server. This is only valid if the server has previously been
--- End diff --
I agree, I was surprised that restart was really just 'start if you've been stopped before'. It doesn't look like any test cases are using this code at the moment, so I'll modify the restart() to do a stop() if it's running, followed by a start().
---
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-114 - Modified the TestingServer to ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/11#discussion_r13859747
--- Diff: curator-test/src/main/java/org/apache/curator/test/TestingServer.java ---
@@ -104,6 +171,17 @@ public void stop() throws IOException
}
/**
+ * Restart the server. This is only valid if the server has previously been
--- End diff --
Why only make this valid if the server had already been stopped? Just for consistency with the internal TestingZookeeperServer behaviour?
---
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-114 - Modified the TestingServer to ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/11
---
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-114 - Modified the TestingServer to ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/11#discussion_r13898859
--- Diff: curator-test/src/main/java/org/apache/curator/test/TestingServer.java ---
@@ -37,47 +37,104 @@
/**
* Create the server using a random port
- *
- * @throws Exception errors
+ *
+ * @throws Exception
+ * errors
*/
public TestingServer() throws Exception
{
+ this(true);
+ }
+
+ /**
+ * Create the server using a random port
+ *
+ * @param start
+ * True if the server should be started, false otherwise
+ * @throws Exception
+ * errors
+ */
+ public TestingServer(boolean start) throws Exception
--- End diff --
Good point, yes it should be passed into the other constructor, will fix this up now.
The start flag is not currently used in any of the unit tests, but I think that it's useful to be able to create a server without starting it. There are cases where you want to start with a server that is not started.
---
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-114 - Modified the TestingServer to ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/11#discussion_r13886728
--- Diff: curator-test/src/main/java/org/apache/curator/test/TestingServer.java ---
@@ -104,6 +171,17 @@ public void stop() throws IOException
}
/**
+ * Restart the server. This is only valid if the server has previously been
--- End diff --
Just following the current convention with the underlying TestingZookeeperServer. It throws an exception if restart() is called on an instance that is not in a STOPPED state. Are you suggesting that restart() should work from any state? Or just a LATENT or STOPPED state? Either way, it would require changes to the TestingZookeeperServer, but I don't think that's inherently a problem as the restart() method is only used by the TestingCluster, and I don't think modifying the behaviour would cause issues there.
---
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-114 - Modified the TestingServer to ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/11#discussion_r13886987
--- Diff: curator-test/src/main/java/org/apache/curator/test/TestingServer.java ---
@@ -104,6 +171,17 @@ public void stop() throws IOException
}
/**
+ * Restart the server. This is only valid if the server has previously been
--- End diff --
As a user, it would make sense for restart to work from any state. My mental model is akin to Unix services where restart is simply "stop ; start"
---
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-114 - Modified the TestingServer to ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/11#discussion_r13898754
--- Diff: curator-test/src/main/java/org/apache/curator/test/TestingServer.java ---
@@ -37,47 +37,104 @@
/**
* Create the server using a random port
- *
- * @throws Exception errors
+ *
+ * @throws Exception
+ * errors
*/
public TestingServer() throws Exception
{
+ this(true);
+ }
+
+ /**
+ * Create the server using a random port
+ *
+ * @param start
+ * True if the server should be started, false otherwise
+ * @throws Exception
+ * errors
+ */
+ public TestingServer(boolean start) throws Exception
--- End diff --
start argument is not actually used. Is it meant to be?
---
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.
---