[ZOOKEEPER-2642] Resurrect the reconfig() methods that were in ZooKeeper.java. #122

Randgalt · 2016-12-10T19:37:21Z

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

….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

hanm · 2016-12-12T21:20:49Z

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.

eribeiro · 2016-12-13T16:39:22Z

src/java/main/org/apache/zookeeper/ZooKeeper.java

-import org.apache.zookeeper.proto.SetDataResponse;
-import org.apache.zookeeper.proto.SyncRequest;
-import org.apache.zookeeper.proto.SyncResponse;
+import org.apache.zookeeper.proto.*;


If I remember well, Patrick Hunt once told me during a code review that we should use explicit imports instead of wildcards. Right, @phunt ?

eribeiro · 2016-12-14T22:46:20Z

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
+    {


nit: inconsistent brace style for method names from line onwards.

…r.java

fpj · 2017-01-11T14:49:35Z

src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml

+
+            <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.


Small typo: reconfig() should be reconfigure().

fpj · 2017-01-11T14:52:23Z

Just so that I understand, when are we going to be removing these deprecated methods, in trunk? I'm asking this for two reasons:

So that users know in which version these methods are going away
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.

hanm · 2017-01-11T21:53:36Z

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.).

hanm · 2017-01-16T06:44:28Z

I think this patch is ready to land... @fpj What do you think?

fpj · 2017-01-16T14:51:18Z

@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.

Randgalt · 2017-01-16T17:27:15Z

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.

Randgalt · 2017-01-16T17:28:11Z

FYI - my PR/patch does rename to reconfigure().

hanm · 2017-01-17T07:22:09Z

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.

fpj · 2017-01-17T10:16:31Z

@hanm @Randgalt sounds like a plan, let's follow the steps that Michael lined up above.

Randgalt · 2017-01-17T13:50:21Z

Will do

Randgalt · 2017-01-17T14:37:24Z

See #151

Randgalt · 2017-01-17T16:51:17Z

Do we need a separate issue for the 3.6 change?

hanm · 2017-01-17T17:40:43Z

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 (#122) so it apply to master, by removing the old reconfig APIs?

Randgalt · 2017-01-17T18:26:47Z

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?

hanm · 2017-01-17T18:35:46Z

@Randgalt Exactly. Thanks!

hanm · 2017-01-17T22:20:34Z

Just to close the loop here:

The PR for master branch is now: #152
The PR for branch-3.5 is now: #151

Was #122 Author: randgalt <[email protected]> Reviewers: Michael Han <[email protected]> Closes #152 from Randgalt/ZOOKEEPER-2642

Was apache#122 Author: randgalt <[email protected]> Reviewers: Michael Han <[email protected]> Closes apache#152 from Randgalt/ZOOKEEPER-2642

Randgalt added 3 commits December 10, 2016 14:33

Add @deprecated annotations

e975fa0

work to make Javadoc sniffer happy

5e8062c

eribeiro reviewed Dec 13, 2016

View reviewed changes

eribeiro reviewed Dec 14, 2016

View reviewed changes

Note deprecated APIs in the doc and fix some style issues in ZooKeepe…

a47bcc6

…r.java

fpj requested changes Jan 11, 2017

View reviewed changes

Randgalt added 2 commits January 11, 2017 15:45

Method name is now reconfigure()

ee483aa

Merge branch 'master' into ZOOKEEPER-2642

4e8b6c6

Randgalt closed this Jan 17, 2017

Randgalt deleted the ZOOKEEPER-2642 branch January 17, 2017 18:42

Randgalt mentioned this pull request Jan 17, 2017

[ZOOKEEPER-2642] reconfig() is now named reconfigure() due to ZOOKEEPER-2642 #152

Closed

asfgit pushed a commit that referenced this pull request Feb 11, 2017

ZOOKEEPER-2642: rename ZooKeeper reconfig API to reconfigure API.

c5df1c9

Was #122 Author: randgalt <[email protected]> Reviewers: Michael Han <[email protected]> Closes #152 from Randgalt/ZOOKEEPER-2642

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZOOKEEPER-2642] Resurrect the reconfig() methods that were in ZooKeeper.java. #122

[ZOOKEEPER-2642] Resurrect the reconfig() methods that were in ZooKeeper.java. #122

Randgalt commented Dec 10, 2016

hanm commented Dec 12, 2016

eribeiro Dec 13, 2016

Randgalt Dec 15, 2016

eribeiro Dec 14, 2016

Randgalt Dec 15, 2016

fpj Jan 11, 2017

fpj commented Jan 11, 2017 •

edited

Loading

hanm commented Jan 11, 2017

hanm commented Jan 16, 2017

fpj commented Jan 16, 2017

Randgalt commented Jan 16, 2017 •

edited

Loading

Randgalt commented Jan 16, 2017

hanm commented Jan 17, 2017

fpj commented Jan 17, 2017

Randgalt commented Jan 17, 2017

Randgalt commented Jan 17, 2017

Randgalt commented Jan 17, 2017

hanm commented Jan 17, 2017

Randgalt commented Jan 17, 2017

hanm commented Jan 17, 2017

hanm commented Jan 17, 2017

[ZOOKEEPER-2642] Resurrect the reconfig() methods that were in ZooKeeper.java. #122

[ZOOKEEPER-2642] Resurrect the reconfig() methods that were in ZooKeeper.java. #122

Conversation

Randgalt commented Dec 10, 2016

hanm commented Dec 12, 2016

eribeiro Dec 13, 2016

Choose a reason for hiding this comment

Randgalt Dec 15, 2016

Choose a reason for hiding this comment

eribeiro Dec 14, 2016

Choose a reason for hiding this comment

Randgalt Dec 15, 2016

Choose a reason for hiding this comment

fpj Jan 11, 2017

Choose a reason for hiding this comment

fpj commented Jan 11, 2017 • edited Loading

hanm commented Jan 11, 2017

hanm commented Jan 16, 2017

fpj commented Jan 16, 2017

Randgalt commented Jan 16, 2017 • edited Loading

Randgalt commented Jan 16, 2017

hanm commented Jan 17, 2017

fpj commented Jan 17, 2017

Randgalt commented Jan 17, 2017

Randgalt commented Jan 17, 2017

Randgalt commented Jan 17, 2017

hanm commented Jan 17, 2017

Randgalt commented Jan 17, 2017

hanm commented Jan 17, 2017

hanm commented Jan 17, 2017

fpj commented Jan 11, 2017 •

edited

Loading

Randgalt commented Jan 16, 2017 •

edited

Loading