You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Randgalt <gi...@git.apache.org> on 2016/12/10 19:37:26 UTC
[GitHub] zookeeper pull request #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
GitHub user Randgalt opened a pull request:
https://github.com/apache/zookeeper/pull/122
[ZOOKEEPER-2642] Resurrect the reconfig() methods that were in ZooKeeper.java.
Resurrect the reconfig() methods that were in ZooKeeper.java. While 3.5.x is an alpha there are clients in production that are relying on the current specification. The reconfig() methods are marked deprecated to inform that they will be removed later. The "new" methods in the new class ZooKeeperAdmin were renamed reconfigure() to avoid getting the deprecation annotation inherited and to provide clarity. Both reconfig() and reconfigure() are implemented via protected methods in ZooKeeper.java
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Randgalt/zookeeper ZOOKEEPER-2642
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/122.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 #122
----
commit 3611d18eb73bac5760293730087d2b0c3241b2bc
Author: randgalt <jo...@jordanzimmerman.com>
Date: 2016-12-10T19:33:58Z
Resurrect the reconfig() methods that were in ZooKeeper.java. While 3.5.x is an alpha there are clients in production that are relying
on the current specification. The reconfig() methods are marked deprecated to inform that they will be removed later. The "new" methods
in the new class ZooKeeperAdmin were renamed reconfigure() to avoid getting the deprecation annotation inherited and to provide clarity.
Both reconfig() and reconfigure() are implemented via protected methods 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 issue #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/122
Will do
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
I think this patch is ready to land... @fpj What do you think?
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/122
> Are you OK with us removing the deprecated methods when 3.5 becomes GA?
No. I'd vote against that. Breaking changes should be in a 3.6 version. That was the genesis of this issue to begin with. Regardless of the intent of the ZK team, 3.5.x has been in production in a large number of companies for years now. To break a public API without increment the version will cause unnecessary problems.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/122#discussion_r92597402
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -39,32 +39,10 @@
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.client.ZooKeeperSaslClient;
import org.apache.zookeeper.common.PathUtils;
+import org.apache.zookeeper.common.StringUtils;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;
-import org.apache.zookeeper.proto.CheckWatchesRequest;
-import org.apache.zookeeper.proto.Create2Response;
-import org.apache.zookeeper.proto.CreateRequest;
-import org.apache.zookeeper.proto.CreateResponse;
-import org.apache.zookeeper.proto.CreateTTLRequest;
-import org.apache.zookeeper.proto.DeleteRequest;
-import org.apache.zookeeper.proto.ExistsRequest;
-import org.apache.zookeeper.proto.GetACLRequest;
-import org.apache.zookeeper.proto.GetACLResponse;
-import org.apache.zookeeper.proto.GetChildren2Request;
-import org.apache.zookeeper.proto.GetChildren2Response;
-import org.apache.zookeeper.proto.GetChildrenRequest;
-import org.apache.zookeeper.proto.GetChildrenResponse;
-import org.apache.zookeeper.proto.GetDataRequest;
-import org.apache.zookeeper.proto.GetDataResponse;
-import org.apache.zookeeper.proto.RemoveWatchesRequest;
-import org.apache.zookeeper.proto.ReplyHeader;
-import org.apache.zookeeper.proto.RequestHeader;
-import org.apache.zookeeper.proto.SetACLRequest;
-import org.apache.zookeeper.proto.SetACLResponse;
-import org.apache.zookeeper.proto.SetDataRequest;
-import org.apache.zookeeper.proto.SetDataResponse;
-import org.apache.zookeeper.proto.SyncRequest;
-import org.apache.zookeeper.proto.SyncResponse;
+import org.apache.zookeeper.proto.*;
--- End diff --
fixed
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
@Randgalt Exactly. Thanks!
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
>> Do we need a separate issue for the 3.6 change?
No we don't (what @fpj prefers wrapping both PR in one shot under ZOOKEEPER-2642).
@Randgalt Do you mind update the current PR (https://github.com/apache/zookeeper/pull/122) so it apply to master, by removing the old reconfig 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 issue #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
>> Breaking changes should be in a 3.6 version.
Sounds OK to me.
>> We need to either create a new issue or have a different pull request for 3.5 under this same issue
I think this pull request (122) should target branch 3.5, because it keeps the old API. We need create a new PR that target master which only contain updates to the API name (reconfig -> reconfigure) and some doc / test updates without bringing back the old APIs.
@Randgalt Could you update the base branch of this PR to be branch-3.5 instead of master? I think this PR is ready to land in branch-3.5.
@fpj I can create a new PR target master that only contain API name changes, if @Randgalt does not get ahead of me on this work.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/122
Do we need a separate issue for the 3.6 change?
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt closed the pull request at:
https://github.com/apache/zookeeper/pull/122
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
Please also update the reconfig documentation about deprecating old API and about the new API in ZooKeeperAdmin:
https://github.com/apache/zookeeper/blob/master/src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml#L299
Other than this, the patch lgtm.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/122#discussion_r92507667
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -2910,4 +2923,49 @@ private ClientCnxnSocket getClientCnxnSocket() throws IOException {
throw ioe;
}
}
+
+ protected byte[] internalReconfig(String joiningServers, String leavingServers, String newMembers, long fromConfig, Stat stat) throws KeeperException, InterruptedException
+ {
--- End diff --
nit: inconsistent brace style for method names from line onwards.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/122
@hanm @Randgalt sounds like a plan, let's follow the steps that Michael lined up above.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/122#discussion_r92597471
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -2910,4 +2923,49 @@ private ClientCnxnSocket getClientCnxnSocket() throws IOException {
throw ioe;
}
}
+
+ protected byte[] internalReconfig(String joiningServers, String leavingServers, String newMembers, long fromConfig, Stat stat) throws KeeperException, InterruptedException
+ {
--- End diff --
fixed
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
Just to close the loop here:
The PR for master branch is now: https://github.com/apache/zookeeper/pull/152
The PR for branch-3.5 is now: https://github.com/apache/zookeeper/pull/151
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/122
Just so that I understand, when are we going to be removing this methods, in trunk? I'm asking this for two reasons:
1. So that users know in which version these methods are going away
2. What changes we need to apply to trunk. In trunk, we would need to at least change in the admin class `reconfig()` to `reconfigure()`, but it sounds like we don't need to bring back the old `reconfig` methods.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/122#discussion_r95591971
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml ---
@@ -300,6 +300,11 @@ server.3=125.23.63.25:2782:2785:participant</programlisting>
from ZooKeeper class, and use of this API requires ACL setup and user
authentication (see <xref linkend="sc_reconfig_access_control"/> for more information.).
</para>
+
+ <para>Note: for temporary backward compatibility, the reconfig() APIs will remain in ZooKeeper.java
+ where they were for a few alpha versions of 3.5.x. However, these APIs are deprecated and users
+ should move to the reconfig() APIs in ZooKeeperAdmin.java.
--- End diff --
Small typo: `reconfig()` should be `reconfigure()`.
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/122
See #151
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/122
>> when are we going to be removing these deprecated methods, in trunk
Maybe when we get a stable release of 3.5?
>> it sounds like we don't need to bring back the old reconfig methods.
Agree, for trunk the change would be just rename ZooKeeperAdmin::reconfig to ZooKeeperAdmin::reconfigure so it's consistent with branch-3.5 (with some documentation updates and tests update.).
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/122
@Randgalt
>> when are we going to be removing these deprecated methods, in trunk
> Maybe when we get a stable release of 3.5?
Are you OK with us removing the deprecated methods when 3.5 becomes GA?
> Agree, for trunk the change would be just rename ZooKeeperAdmin::reconfig to ZooKeeperAdmin::reconfigure so it's consistent with branch-3.5 (with some documentation updates and tests update.).
We need to either create a new issue or have a different pull request for 3.5 under this same issue. I'd rather do the latter just so that we wrap this up in one shot, but it depends on whether @Randgalt is willing to do 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] zookeeper issue #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/122
OK - to be clear, I have PR #151 which applies the legacy API to branch-3.5. Then, I'll rework _this_ PR (against master) to only have the Admin API, right?
---
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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/122#discussion_r92209798
--- Diff: src/java/main/org/apache/zookeeper/ZooKeeper.java ---
@@ -39,32 +39,10 @@
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.client.ZooKeeperSaslClient;
import org.apache.zookeeper.common.PathUtils;
+import org.apache.zookeeper.common.StringUtils;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;
-import org.apache.zookeeper.proto.CheckWatchesRequest;
-import org.apache.zookeeper.proto.Create2Response;
-import org.apache.zookeeper.proto.CreateRequest;
-import org.apache.zookeeper.proto.CreateResponse;
-import org.apache.zookeeper.proto.CreateTTLRequest;
-import org.apache.zookeeper.proto.DeleteRequest;
-import org.apache.zookeeper.proto.ExistsRequest;
-import org.apache.zookeeper.proto.GetACLRequest;
-import org.apache.zookeeper.proto.GetACLResponse;
-import org.apache.zookeeper.proto.GetChildren2Request;
-import org.apache.zookeeper.proto.GetChildren2Response;
-import org.apache.zookeeper.proto.GetChildrenRequest;
-import org.apache.zookeeper.proto.GetChildrenResponse;
-import org.apache.zookeeper.proto.GetDataRequest;
-import org.apache.zookeeper.proto.GetDataResponse;
-import org.apache.zookeeper.proto.RemoveWatchesRequest;
-import org.apache.zookeeper.proto.ReplyHeader;
-import org.apache.zookeeper.proto.RequestHeader;
-import org.apache.zookeeper.proto.SetACLRequest;
-import org.apache.zookeeper.proto.SetACLResponse;
-import org.apache.zookeeper.proto.SetDataRequest;
-import org.apache.zookeeper.proto.SetDataResponse;
-import org.apache.zookeeper.proto.SyncRequest;
-import org.apache.zookeeper.proto.SyncResponse;
+import org.apache.zookeeper.proto.*;
--- End diff --
If I remember well, Patrick Hunt once told me during a code review that we should use explicit imports instead of wildcards. Right, @phunt ?
---
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.
---