ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-544

improve client testability - allow test client to access connected server location

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.0
    • Component/s: c client, java client, tests
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This came up recently on the user list. If you are developing tests for your zk client you need to be able to access the server that your
      session is currently connected to. The reason is that your test needs to know which server in the quorum to shutdown in order to
      verify you are handling failover correctly. Similar for session expiration testing.

      however we should be careful, we prefer not to expose this to all clients, this is an implementation detail that we typically
      want to hide.

      also we should provide this in both the c and java clients

      I suspect we should add a protected method on ZooKeeper. This will make a higher bar (user will have to subclass) for
      the user to access this method. In tests it's fine, typically you want a "TestableZooKeeper" class anyway. In c we unfortunately
      have less options, we can just rely on docs for now.

      In both cases (c/java) we need to be very very clear in the docs that this is for testing only and to clearly define semantics.

      We should add the following at the same time:

      toString() method to ZooKeeper which includes server ip/port, client port, any other information deemed useful (connection stats like send/recv?)

      the java ZooKeeper is missing "deterministic connection order" that the c client has. this is also useful for testing. again, protected and
      clear docs that this is for testing purposes only!

      Any other things we should expose?

      1. ZOOKEEPER-544.patch
        11 kB
        Mahadev konar
      2. ZOOKEEPER-544.patch
        11 kB
        Mahadev konar
      3. ZOOKEEPER-544.patch
        12 kB
        Patrick Hunt

        Activity

        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #655 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/655/)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #655 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/655/ )
        Hide
        Benjamin Reed added a comment -

        Committed revision 896143.

        Show
        Benjamin Reed added a comment - Committed revision 896143.
        Hide
        Hadoop QA added a comment -

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

        +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/96/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/96/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/96/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/12428459/ZOOKEEPER-544.patch against trunk revision 892111. +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/96/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/96/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/96/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        screwed up the last patch. upload a new one and trying hudson again!

        Show
        Mahadev konar added a comment - screwed up the last patch. upload a new one and trying hudson again!
        Hide
        Hadoop QA added a comment -

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

        +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 appears to cause Findbugs to fail.

        +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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/95/testReport/
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/95/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/12428405/ZOOKEEPER-544.patch against trunk revision 892111. +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 appears to cause Findbugs to fail. +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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/95/testReport/ Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/95/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        updating the patch to trunk. Resolved the conflict.

        Pat, can you check if the patch is all fine and I didnt miss anything.

        Show
        Mahadev konar added a comment - updating the patch to trunk. Resolved the conflict. Pat, can you check if the patch is all fine and I didnt miss anything.
        Hide
        Mahadev konar added a comment -

        updating the patch to trunk.

        Show
        Mahadev konar added a comment - updating the patch to trunk.
        Hide
        Hadoop QA added a comment -

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

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

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

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

        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/94/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/12424336/ZOOKEEPER-544.patch against trunk revision 892111. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/94/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        trying hudson out agian, after all the changes went in....

        Show
        Mahadev konar added a comment - trying hudson out agian, after all the changes went in....
        Hide
        Patrick Hunt added a comment -

        discoverability. I put testable* to make it easier for clients to do test (rather than figure out how to get this).

        Also, exposing cnxn means it's harder to make changes to the implementation as the user code
        will be closely coupled to the guts of cnxn. this allows for changes under the covers (for common
        use cases at least)

        Show
        Patrick Hunt added a comment - discoverability. I put testable* to make it easier for clients to do test (rather than figure out how to get this). Also, exposing cnxn means it's harder to make changes to the implementation as the user code will be closely coupled to the guts of cnxn. this allows for changes under the covers (for common use cases at least)
        Hide
        Benjamin Reed added a comment -

        why do you have testableLocalSocketAddress in ZooKeeper? The cnxn object is protected. You don't need that method is ZooKeeper do you?

        Show
        Benjamin Reed added a comment - why do you have testableLocalSocketAddress in ZooKeeper? The cnxn object is protected. You don't need that method is ZooKeeper do you?
        Hide
        Hadoop QA added a comment -

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

        +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/57/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/57/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/57/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/12424336/ZOOKEEPER-544.patch against trunk revision 833938. +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/57/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/57/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/57/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Added some methods to the java to improve testability - in particular to
        get the tostring of zk object and allow access to local/report ip/port. Also
        added some stats for tracking sent/recv information - included in tostring.
        Added tests of these features - see the log output for examples.

        Note to reviewer - be sure to check some cleanups I did in the code surrounding
        the send/recv counts. Note my attempt to inform potential users that these
        methods are meant for testing by 1) making them protected, 2) naming, 3)
        javadoc comments.

        Also, it would be great to get c versions of these 3 new methods. Ben?

        Show
        Patrick Hunt added a comment - Added some methods to the java to improve testability - in particular to get the tostring of zk object and allow access to local/report ip/port. Also added some stats for tracking sent/recv information - included in tostring. Added tests of these features - see the log output for examples. Note to reviewer - be sure to check some cleanups I did in the code surrounding the send/recv counts. Note my attempt to inform potential users that these methods are meant for testing by 1) making them protected, 2) naming, 3) javadoc comments. Also, it would be great to get c versions of these 3 new methods. Ben?

          People

          • Assignee:
            Patrick Hunt
            Reporter:
            Patrick Hunt
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development