You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by afine <gi...@git.apache.org> on 2017/01/05 21:27:51 UTC
[GitHub] zookeeper pull request #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooK...
GitHub user afine opened a pull request:
https://github.com/apache/zookeeper/pull/143
ZOOKEEPER-2511: Implement AutoCloseable in ZooKeeper.java
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/afine/zookeeper ZOOKEEPER-2511
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/143.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 #143
----
commit 5ef495483a080c390ac512e602014f0937b1fd15
Author: Abraham Fine <af...@apache.org>
Date: 2017-01-05T21:26:26Z
ZOOKEEPER-2511: Implement AutoCloseable in ZooKeeper.java
----
---
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 #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooK...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/143#discussion_r94995400
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -127,7 +127,13 @@
* EventNone and state sKeeperStateDisconnected.
*
*/
-public class ZooKeeper {
+// We suppress the "try" warning here because the close() method's signature
--- End diff --
I used // comments to clarify the separation between this comment and the javadoc (since I believe the information is only important when compiling zookeeper). Let me know if you think that is valid.
---
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 #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooK...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/143
---
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 #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooKeeper.j...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/143
LGTM, merging, thanks @afine.
---
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 #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooK...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/143#discussion_r95032230
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -127,7 +127,13 @@
* EventNone and state sKeeperStateDisconnected.
*
*/
-public class ZooKeeper {
+// We suppress the "try" warning here because the close() method's signature
--- End diff --
Good point on avoiding mixing implementation comments with end user facing documents. Though in this case it is safe to use block comments as @eribeiro pointed out because block comments (enclosed in '/* */') will not be part of java doc (enclosed in '/** */'), also use block comments provide better visual consistency style with reset of multi line comments in src file.
It might also be helpful to add to Java doc on ZooKeeper.close so user aware that since 3.5.3 they could optionally use try-with-resource on ZooKeeper object without explicitly invoking close 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] zookeeper pull request #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooK...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/143#discussion_r95219646
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -127,7 +127,13 @@
* EventNone and state sKeeperStateDisconnected.
*
*/
-public class ZooKeeper {
+// We suppress the "try" warning here because the close() method's signature
--- End diff --
agreed, let me know if the changes address your concerns @eribeiro @hanm
---
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 #143: ZOOKEEPER-2511: Implement AutoCloseable in ZooK...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/143#discussion_r94860657
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -127,7 +127,13 @@
* EventNone and state sKeeperStateDisconnected.
*
*/
-public class ZooKeeper {
+// We suppress the "try" warning here because the close() method's signature
--- End diff --
Nit: use multiline comments /* */
---
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.
---