You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by eolivelli <gi...@git.apache.org> on 2017/04/11 13:42:31 UTC
[GitHub] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
GitHub user eolivelli opened a pull request:
https://github.com/apache/zookeeper/pull/222
ZOOKEEPER-2697 Handle graceful stop of ZookKeeper client
Add a ZooKeeper.close(int timeout) method which waits for internal resources to be released
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eolivelli/zookeeper ZOOKEEPER-2697-close-with-timeout
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/222.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 #222
----
commit 509f5a396496e409827bfed8a73a7bb2e5b62534
Author: eolivelli <eo...@apache.org>
Date: 2017-04-11T13:41:06Z
ZOOKEEPER-2697 Handle graceful stop of ZookKeeper client
----
---
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] zookeeper issue #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeeper clie...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/222
Someone can correct me if I were wrong but I don't think we can cherry pick this to branch-3.5, which is in beta state now. A beta state means API is locked down: no addition, removal, or change of APIs.
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r110944401
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeout timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeout) throws InterruptedException {
--- End diff --
pet peeve - timing argument names that don't imply the unit. Can you name this `waitForShutdownTimeoutMs`?
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113414762
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeoutMs) throws InterruptedException {
+ close();
+ if (waitForShutdownTimeoutMs > 0) {
+ return testableWaitForShutdown(waitForShutdownTimeoutMs);
--- End diff --
@hanm I did not change the testableWaitForShutdown method in order not to break binary compatibilty with existing subclasses.
For instance in my projects I use that method in order to achieve the purpose of this patches,
If it is not a problem for other users I will change the signature
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113008213
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeoutMs) throws InterruptedException {
+ close();
+ if (waitForShutdownTimeoutMs > 0) {
+ return testableWaitForShutdown(waitForShutdownTimeoutMs);
--- End diff --
testableWaitForShutdown should be renamed to something like waitForShutDown - it's not a method used in test anymore.
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/222
---
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] zookeeper issue #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeeper clie...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/222
@hanm thank you for your review.
I have added a javadoc note on the close() method and the @since tag.
Thank for including this in the 3.5.4 release 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] zookeeper issue #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeeper clie...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/222
@hanm This patch only adds a new method, with no changes to the rest of the library and it does not break binary compatibility.
So IMHO It can be safely added to 3.5.x branch to. But I am a newbie of ZK project rules.
---
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] zookeeper issue #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeeper clie...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/222
@Randgalt @hanm can you review please ?
Wish: can this patch be merged in 3.5 branch too ?
@Randgalt I will be happy to submit a PR for curator once this new method gets into 3.5
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113008830
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeoutMs) throws InterruptedException {
+ close();
+ if (waitForShutdownTimeoutMs > 0) {
--- End diff --
I don't think this check is needed - actually, it sounds better to remove this check so a negative value will trigger illegal argument exception indicates something is wrong in upstream code, because it's hard to know if the negative value is passed on purpose, or is an accident.
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113902401
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,20 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
--- End diff --
@hanm 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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113753479
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,20 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
--- End diff --
Please also add a @since 3.5.4 tag in the doc comment here.
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113415006
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeoutMs) throws InterruptedException {
--- End diff --
synchronization is in the real close() method. IMHO If I will add the synch here I think we will introduce unpredictable side effects
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r111136880
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeout timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeout) throws InterruptedException {
--- End diff --
@Randgalt Thank you for the review
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] zookeeper issue #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeeper clie...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/222
Since we have this new close method we probably should remove the testableClose but it looks like there are also other testable* methods since 3.3.0 and we have to consider there are many folks already use this testable method (even not just in test code.). So it sounds good to me to keep the testable method as is and because of this it also sounds good to me to add the new close method to 3.5.
I think what's remaining is some doc work:
* Add @since tag in the new close method.
* Would be good to some comments in the existing close() method mentioning that it does not release all threads internally used, and then link to the new close method added in this patch if users want certain behavior.
---
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] zookeeper pull request #222: ZOOKEEPER-2697 Handle graceful stop of ZookKeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/222#discussion_r113008904
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -1325,6 +1325,23 @@ public synchronized void close() throws InterruptedException {
}
/**
+ * Close this client object as the {@link #close() } method.
+ * This method will wait for internal resources to be released.
+ *
+ * @param waitForShutdownTimeoutMs timeout (in milliseconds) to wait for resources to be released.
+ * Use zero or a negative value to skip the wait
+ * @throws InterruptedException
+ * @return true if waitForShutdownTimeout is greater than zero and all of the resources have been released
+ */
+ public boolean close(int waitForShutdownTimeoutMs) throws InterruptedException {
--- End diff --
no synchronization here?
---
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.
---