ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1172

Support for custom org.apache.zookeeper.client.HostProvider implementation.

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.6.0
    • Component/s: java client
    • Labels:
      None
    • Release Note:
      Support for custom org.apache.zookeeper.client.HostProvider implementation with the help of new Zookeeper constructor methods.

      Description

      The interface org.apache.zookeeper.client.HostProvider exist but it is hardcoded to org.apache.zookeeper.client.StaticHostProvider at Zookeeper constructor.

      Now it could be replaced by any other implementation just by calling the new Zookeeper constructor methods which accept a HostProvider as paramater.

      1. ZOOKEEPER-1172.patch
        34 kB
        César Álvarez Núñez
      2. ZOOKEEPER-1172.patch
        35 kB
        César Álvarez Núñez
      3. ZOOKEEPER-1172.patch
        35 kB
        César Álvarez Núñez

        Activity

        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12493158/ZOOKEEPER-1172.patch
        against trunk revision 1165443.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 18 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/506//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/506//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/506//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12493158/ZOOKEEPER-1172.patch against trunk revision 1165443. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/506//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/506//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/506//console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        cancelling patch, a few issues to be resolved:

        1) you need to grant the change to ASF (in attach screen), otw we can't accept it, in this case denied (see "manage attachments" under attachments)

        2) some issues with qa bot - findbugs and core test issues.

        3) don't use system properties/env, rather use config file

        4) docs (src/docs) need to be updated

        Show
        Patrick Hunt added a comment - cancelling patch, a few issues to be resolved: 1) you need to grant the change to ASF (in attach screen), otw we can't accept it, in this case denied (see "manage attachments" under attachments) 2) some issues with qa bot - findbugs and core test issues. 3) don't use system properties/env, rather use config file 4) docs (src/docs) need to be updated
        Hide
        César Álvarez Núñez added a comment -

        Sorry, I should have read the link https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute How To Contribute more carefully.

        Fixed findbugs and core test issues. Running full "ant test".

        With regard to "don't use system properties/env, rather use config file", I'm not really sure if config file is the best option since it contains server configuration properties and not client properties.

        Anyway I think that a better approach is to include a new constructor like the following (with its corresponding two sibling without 'canBeReadOnly' and 'sessionId & sessionPasswd'.):

        ZooKeeper.java
            public ZooKeeper(HostProvider hostProvider,
                         String chroot,
                         int sessionTimeout,
                         Watcher watcher,
                         long sessionId,
                         byte[] sessionPasswd,
                         boolean canBeReadOnly) throws IOException
        

        Where "connectString" is replaced by two new parameters: HostProvider and chroot.
        In fact "chroot" should be separated from "servers list" because you can interpret that it is allowed to set a chroot per server (server1:port1/chroot1,server2:port2/chroot2).

        I understand that there is not any limitation on adding new constructors as long as backward compatibility is not broken, don't I?

        Finally, why priority has been changed to "Major"? I don't like it could block in some way the 3.4.0 release.

        Show
        César Álvarez Núñez added a comment - Sorry, I should have read the link https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute How To Contribute more carefully. Fixed findbugs and core test issues. Running full "ant test". With regard to "don't use system properties/env, rather use config file", I'm not really sure if config file is the best option since it contains server configuration properties and not client properties. Anyway I think that a better approach is to include a new constructor like the following (with its corresponding two sibling without 'canBeReadOnly' and 'sessionId & sessionPasswd'.): ZooKeeper.java public ZooKeeper(HostProvider hostProvider, String chroot, int sessionTimeout, Watcher watcher, long sessionId, byte [] sessionPasswd, boolean canBeReadOnly) throws IOException Where "connectString" is replaced by two new parameters: HostProvider and chroot. In fact "chroot" should be separated from "servers list" because you can interpret that it is allowed to set a chroot per server (server1:port1/chroot1,server2:port2/chroot2). I understand that there is not any limitation on adding new constructors as long as backward compatibility is not broken, don't I? Finally, why priority has been changed to "Major"? I don't like it could block in some way the 3.4.0 release.
        Hide
        César Álvarez Núñez added a comment -

        New approach based on new Zookeeper constructors.

        Show
        César Álvarez Núñez added a comment - New approach based on new Zookeeper constructors.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494080/ZOOKEEPER-1172.patch
        against trunk revision 1166970.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 6 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/527//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/527//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/527//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12494080/ZOOKEEPER-1172.patch against trunk revision 1166970. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/527//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/527//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/527//console This message is automatically generated.
        Hide
        Thomas Koch added a comment -

        Could you please upload the patch to https://reviews.apache.org/r/new/ for review?

        Show
        Thomas Koch added a comment - Could you please upload the patch to https://reviews.apache.org/r/new/ for review?
        Hide
        César Álvarez Núñez added a comment -

        Done. Since it is my first time please let me know if I've missed something.

        Show
        César Álvarez Núñez added a comment - Done. Since it is my first time please let me know if I've missed something.
        Hide
        Thomas Koch added a comment -

        The review system should have posted a comment here in Jira with a link to the review request. You missed to specify the number of this issue when you created the review request.
        (This whole setup is not perfect... http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361 )

        Show
        Thomas Koch added a comment - The review system should have posted a comment here in Jira with a link to the review request. You missed to specify the number of this issue when you created the review request. (This whole setup is not perfect... http://permalink.gmane.org/gmane.comp.apache.infrastructure.devel/1361 )
        Hide
        Thomas Koch added a comment -

        The issue number in ReviewBoard should just be ZOOKEEPER-XYZ, not the full URL.

        Show
        Thomas Koch added a comment - The issue number in ReviewBoard should just be ZOOKEEPER-XYZ, not the full URL.
        Hide
        Benjamin Reed added a comment -

        this is great functionality.

        @phunt i'm not sure why you are objecting to a system property. i think it is much better than putting it in the constructors.

        doing a chroot per server will not work. the trees in each replica must be the same, so using different roots for servers will cause problems. (it's also separate functionality that shouldn't be in this jira.)

        Show
        Benjamin Reed added a comment - this is great functionality. @phunt i'm not sure why you are objecting to a system property. i think it is much better than putting it in the constructors. doing a chroot per server will not work. the trees in each replica must be the same, so using different roots for servers will cause problems. (it's also separate functionality that shouldn't be in this jira.)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12496938/ZOOKEEPER-1172.patch
        against trunk revision 1177042.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 9 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/595//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/595//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/595//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12496938/ZOOKEEPER-1172.patch against trunk revision 1177042. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/595//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/595//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/595//console This message is automatically generated.
        Hide
        César Álvarez Núñez added a comment -

        Fixed FindBug and TestCase.

        Show
        César Álvarez Núñez added a comment - Fixed FindBug and TestCase.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12497042/ZOOKEEPER-1172.patch
        against trunk revision 1177432.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 9 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/598//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/598//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/598//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12497042/ZOOKEEPER-1172.patch against trunk revision 1177432. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/598//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/598//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/598//console This message is automatically generated.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1977/
        -----------------------------------------------------------

        (Updated 2011-10-05 12:00:44.407863)

        Review request for zookeeper.

        Summary
        -------

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1977/ ----------------------------------------------------------- (Updated 2011-10-05 12:00:44.407863) Review request for zookeeper. Summary -------
        Hide
        Gunnar Wagenknecht added a comment -

        Some feedback on that patch (note, I'm not a ZK committer).

        • I don't like ZooConstructorParams. I'd prefer just adding a new constructor to ZooKeeper. The requested builder pattern can be implemented on top of that, i.e. a ZooKeeperBuilder that just invokes that appropriate ZooKeeper constructor when done.
        • Removing a public method in a public class (eg., ConnectStringParser) is a breaking API change (if that class is considered API). BTW, adding new constructors or methods to classes is no breaking change and doable for a x.y+1 release.
        Show
        Gunnar Wagenknecht added a comment - Some feedback on that patch (note, I'm not a ZK committer). I don't like ZooConstructorParams . I'd prefer just adding a new constructor to ZooKeeper. The requested builder pattern can be implemented on top of that, i.e. a ZooKeeperBuilder that just invokes that appropriate ZooKeeper constructor when done. Removing a public method in a public class (eg., ConnectStringParser ) is a breaking API change (if that class is considered API). BTW, adding new constructors or methods to classes is no breaking change and doable for a x.y+1 release.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12497042/ZOOKEEPER-1172.patch
        against trunk revision 1586200.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 9 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2033//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12497042/ZOOKEEPER-1172.patch against trunk revision 1586200. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2033//console This message is automatically generated.

          People

          • Assignee:
            César Álvarez Núñez
            Reporter:
            César Álvarez Núñez
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development