ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1355

Add zk.updateServerList(newServerList)

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: c client, java client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When the set of servers changes, we would like to update the server list stored by clients without restarting the clients.
      Moreover, assuming that the number of clients per server is the same (in expectation) in the old configuration (as guaranteed by the current list shuffling for example), we would like to re-balance client connections across the new set of servers in a way that a) the number of clients per server is the same for all servers (in expectation) and b) there is no excessive/unnecessary client migration.

      It is simple to achieve (a) without (b) - just re-shuffle the new list of servers at every client. But this would create unnecessary migration, which we'd like to avoid.

      We propose a simple probabilistic migration scheme that achieves (a) and (b) - each client locally decides whether and where to migrate when the list of servers changes. The attached document describes the scheme and shows an evaluation of it in Zookeeper. We also implemented re-balancing through a consistent-hashing scheme and show a comparison. We derived the probabilistic migration rules from a simple formula that we can also provide, if someone's interested in the proof.

      1. loadbalancing.pdf
        288 kB
        Alexander Shraer
      2. loadbalancing-more-details.pdf
        301 kB
        Alexander Shraer
      3. ZOOKEEPER=1355-ver3.patch
        24 kB
        Alexander Shraer
      4. ZOOKEEPER-1355-10-Oct.patch
        89 kB
        Alexander Shraer
      5. ZOOKEEPER-1355-12-Oct.patch
        120 kB
        Marshall McMullen
      6. ZOOKEEPER-1355-13-Nov.patch
        121 kB
        Marshall McMullen
      7. ZOOKEEPER-1355-13-Nov-ver2.patch
        121 kB
        Marshall McMullen
      8. ZOOKEEPER-1355-13-Oct.patch
        119 kB
        Marshall McMullen
      9. ZOOKEEPER-1355-6-Nov.patch
        120 kB
        Marshall McMullen
      10. ZOOKEEPER-1355-6-Nov-ver2.patch
        120 kB
        Marshall McMullen
      11. ZOOKEEPER-1355-ver10.patch
        113 kB
        Marshall McMullen
      12. ZOOKEEPER-1355-ver10-1.patch
        114 kB
        Marshall McMullen
      13. ZOOKEEPER-1355-ver10-2.patch
        114 kB
        Marshall McMullen
      14. ZOOKEEPER-1355-ver10-3.patch
        241 kB
        Marshall McMullen
      15. ZOOKEEPER-1355-ver10-4.patch
        121 kB
        Marshall McMullen
      16. ZOOKEEPER-1355-ver10-4.patch
        114 kB
        Marshall McMullen
      17. ZOOKEEPER-1355-ver11.patch
        121 kB
        Marshall McMullen
      18. ZOOKEEPER-1355-ver11-1.patch
        121 kB
        Marshall McMullen
      19. ZOOKEEPER-1355-ver12.patch
        121 kB
        Marshall McMullen
      20. ZOOKEEPER-1355-ver12-1.patch
        120 kB
        Marshall McMullen
      21. ZOOKEEPER-1355-ver12-2.patch
        120 kB
        Marshall McMullen
      22. ZOOKEEPER-1355-ver12-4.patch
        119 kB
        Marshall McMullen
      23. ZOOKEEPER-1355-ver13.patch
        119 kB
        Marshall McMullen
      24. ZOOKEEPER-1355-ver14.patch
        119 kB
        Marshall McMullen
      25. ZOOKEEPER-1355-ver2.patch
        22 kB
        Alexander Shraer
      26. ZOOKEEPER-1355-ver4.patch
        27 kB
        Alexander Shraer
      27. ZOOKEEPER-1355-ver5.patch
        27 kB
        Alexander Shraer
      28. ZOOKEEPER-1355-ver6.patch
        32 kB
        Alexander Shraer
      29. ZOOKEEPER-1355-ver7.patch
        32 kB
        Alexander Shraer
      30. ZOOKEEPER-1355-ver8.patch
        33 kB
        Alexander Shraer
      31. ZOOKEEPER-1355-ver9.patch
        34 kB
        Alexander Shraer
      32. ZOOKEEPER-1355-ver9-1.patch
        34 kB
        Alexander Shraer
      33. ZOOOKEEPER-1355.patch
        22 kB
        Alexander Shraer
      34. ZOOOKEEPER-1355-test.patch
        24 kB
        Alexander Shraer
      35. ZOOOKEEPER-1355-ver1.patch
        22 kB
        Alexander Shraer

        Issue Links

          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/12510089/ZOOOKEEPER-1355.patch
          against trunk revision 1227927.

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

          +1 tests included. The patch appears to include 3 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/895//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/895//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/895//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/12510089/ZOOOKEEPER-1355.patch against trunk revision 1227927. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/895//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/895//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/895//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12510108/ZOOOKEEPER-1355-ver1.patch
          against trunk revision 1227927.

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

          +1 tests included. The patch appears to include 3 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/896//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/896//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/896//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/12510108/ZOOOKEEPER-1355-ver1.patch against trunk revision 1227927. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/896//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/896//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/896//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12510121/ZOOOKEEPER-1355-test.patch
          against trunk revision 1227927.

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

          +1 tests included. The patch appears to include 3 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/897//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/897//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/897//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/12510121/ZOOOKEEPER-1355-test.patch against trunk revision 1227927. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/897//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/897//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/897//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12510149/ZOOKEEPER-1355-ver2.patch
          against trunk revision 1227927.

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

          +1 tests included. The patch appears to include 3 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/898//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/898//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/898//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/12510149/ZOOKEEPER-1355-ver2.patch against trunk revision 1227927. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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/898//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/898//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/898//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          this looks really good! i have a couple of requests and something to think about:

          1) you need to use spaces rather than tabs. you have a bunch of tabs in the patch. (there is a setting in eclipse to do tabs rather than spaces)
          2) it would be nice in updateList to comment about the overall strategy for determining the return code
          3) can you add a comment to "(currentIndex >= serverAddresses.size())" to say why >= is needed? i think i see it in the context of this patch, but later people might be tempted to change it back to ==

          something to think about: on the updateServerList method, perhaps rather than making the second parameter a boolean we can make it the InetSocketAddress of the currently connected server. what do you think? i don't feel strongly about it.

          Show
          Benjamin Reed added a comment - this looks really good! i have a couple of requests and something to think about: 1) you need to use spaces rather than tabs. you have a bunch of tabs in the patch. (there is a setting in eclipse to do tabs rather than spaces) 2) it would be nice in updateList to comment about the overall strategy for determining the return code 3) can you add a comment to "(currentIndex >= serverAddresses.size())" to say why >= is needed? i think i see it in the context of this patch, but later people might be tempted to change it back to == something to think about: on the updateServerList method, perhaps rather than making the second parameter a boolean we can make it the InetSocketAddress of the currently connected server. what do you think? i don't feel strongly about it.
          Hide
          Alexander Shraer added a comment -

          Thanks, Ben.

          I posted an updated patch that addresses all your comments. I don't remember why I changed the == to >=, seems unneeded as I set currentIndex to -1 with each update (perhaps this stuck from an earlier version of the code). Anyway I removed this change.

          Show
          Alexander Shraer added a comment - Thanks, Ben. I posted an updated patch that addresses all your comments. I don't remember why I changed the == to >=, seems unneeded as I set currentIndex to -1 with each update (perhaps this stuck from an earlier version of the code). Anyway I removed this change.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12510231/ZOOKEEPER%3D1355-ver3.patch
          against trunk revision 1227927.

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

          +1 tests included. The patch appears to include 3 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/899//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/899//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/899//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/12510231/ZOOKEEPER%3D1355-ver3.patch against trunk revision 1227927. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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/899//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/899//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/899//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          some more details added to the design document.

          Would be great if more committers comment on this patch.

          thanks a lot,
          alex

          Show
          Alexander Shraer added a comment - some more details added to the design document. Would be great if more committers comment on this patch. thanks a lot, alex
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12510782/loadbalancing-more-details.pdf
          against trunk revision 1231809.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/908//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/12510782/loadbalancing-more-details.pdf against trunk revision 1231809. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/908//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          can you freshen the patch alex?

          Show
          Benjamin Reed added a comment - can you freshen the patch alex?
          Hide
          Benjamin Reed added a comment -

          ah duh, sorry alex, that was the pdf.

          ok, a couple more things:

          1) can you fix the indentation of watchmanager in zookeeper.java
          2) nextInReconfig needs to be private and a function comment just to explain why "InReconfig" is different
          3) updateServerList needs to be thread safe. with this call there are fields: serverList, reconfig, and indices that are being accessed by the connection thread and the thread calling updateServerList.

          Show
          Benjamin Reed added a comment - ah duh, sorry alex, that was the pdf. ok, a couple more things: 1) can you fix the indentation of watchmanager in zookeeper.java 2) nextInReconfig needs to be private and a function comment just to explain why "InReconfig" is different 3) updateServerList needs to be thread safe. with this call there are fields: serverList, reconfig, and indices that are being accessed by the connection thread and the thread calling updateServerList.
          Hide
          Alexander Shraer added a comment -

          ZOOKEEPER-1355-ver4.patch addresses Ben's comments. The synchronization now allows multiple threads updating the server list and another single thread trying to connect (calling next) as before.

          Show
          Alexander Shraer added a comment - ZOOKEEPER-1355 -ver4.patch addresses Ben's comments. The synchronization now allows multiple threads updating the server list and another single thread trying to connect (calling next) as before.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511608/ZOOKEEPER-1355-ver4.patch
          against trunk revision 1234974.

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

          +1 tests included. The patch appears to include 3 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 passed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/914//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/914//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/914//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/12511608/ZOOKEEPER-1355-ver4.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/914//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/914//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/914//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511612/ZOOKEEPER-1355-ver5.patch
          against trunk revision 1234974.

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

          +1 tests included. The patch appears to include 3 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/915//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/915//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/915//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/12511612/ZOOKEEPER-1355-ver5.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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/915//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/915//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/915//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          +1 i think this is ready to commit. any objections?

          Show
          Benjamin Reed added a comment - +1 i think this is ready to commit. any objections?
          Hide
          Mahadev konar added a comment -

          Ben,
          I was taking a look at it. Mind waiting till tomm?

          Show
          Mahadev konar added a comment - Ben, I was taking a look at it. Mind waiting till tomm?
          Hide
          Mahadev konar added a comment -

          Ben/Alex,
          This adds 2 public api to zookeeper handle (java). Is this intended? What the intent of getCurrentHost?

          Also, I looked at the pdf (which scares me a little - hate looking at all the math symbols ). Can you please explain in layman terms what the process for the client to select the server to connect to? What if the server list is incorrect, what happens then?

          Show
          Mahadev konar added a comment - Ben/Alex, This adds 2 public api to zookeeper handle (java). Is this intended? What the intent of getCurrentHost? Also, I looked at the pdf (which scares me a little - hate looking at all the math symbols ). Can you please explain in layman terms what the process for the client to select the server to connect to? What if the server list is incorrect, what happens then?
          Hide
          Mahadev konar added a comment -

          One more thing, what about the c client? Will we be seeing similar changes to c client? I'd very much like to keep both of them in sync if possible. We are already a little different given the security patches.

          Show
          Mahadev konar added a comment - One more thing, what about the c client? Will we be seeing similar changes to c client? I'd very much like to keep both of them in sync if possible. We are already a little different given the security patches.
          Hide
          Patrick Hunt added a comment -

          Good points Mahadev.

          I don't see any documentation in the guides - we should provide some information there as well for users.

          updateServerList is missing javadoc

          also getCurrentHost (also missing javadoc) is pretty ambiguous name. In the past we haven't wanted to expose the remote server details to the client.

          how is getCurrentHost different from testableRemoteSocketAddress ?

          Show
          Patrick Hunt added a comment - Good points Mahadev. I don't see any documentation in the guides - we should provide some information there as well for users. updateServerList is missing javadoc also getCurrentHost (also missing javadoc) is pretty ambiguous name. In the past we haven't wanted to expose the remote server details to the client. how is getCurrentHost different from testableRemoteSocketAddress ?
          Hide
          Benjamin Reed added a comment -

          i'll let alex provide a better answer, but just to test my understanding:

          the scary math symbols boil down to a simple set of 4 rules for maintaining balanced load when the serverList changes. (the rules are at the end of the document) if you try to do something naive, like randomly disconnecting from the server you are connecting to, it will cause lots of reconnects and you still will end up with imbalanced load. it all boils down to four simple rules for determining when to disconnect from your current host. the math is just there so that it is clear that it wasn't pulled from thin air.

          as far as the public api. we have to add the updateServerList since that is the whole point of the bug. i have no strong feelings about getCurrentHost() in terms of the name or making it public. it is only different from testableRemoteSocketAddress in the sense that testableRemoteSocketAddress says that it should only be used for testing in the javadoc. alex should add javadoc to those two methods.

          i agree that we should also get a C client. alex are you willing to do that? can we do it in a separate jira?

          Show
          Benjamin Reed added a comment - i'll let alex provide a better answer, but just to test my understanding: the scary math symbols boil down to a simple set of 4 rules for maintaining balanced load when the serverList changes. (the rules are at the end of the document) if you try to do something naive, like randomly disconnecting from the server you are connecting to, it will cause lots of reconnects and you still will end up with imbalanced load. it all boils down to four simple rules for determining when to disconnect from your current host. the math is just there so that it is clear that it wasn't pulled from thin air. as far as the public api. we have to add the updateServerList since that is the whole point of the bug. i have no strong feelings about getCurrentHost() in terms of the name or making it public. it is only different from testableRemoteSocketAddress in the sense that testableRemoteSocketAddress says that it should only be used for testing in the javadoc. alex should add javadoc to those two methods. i agree that we should also get a C client. alex are you willing to do that? can we do it in a separate jira?
          Hide
          Alexander Shraer added a comment -

          Hi, thanks for the comments!! I removed getCurrentHost, added a JavaDoc for updateServerList, and a paragraph to the documentation.

          I can do the C client changes (although, after looking at the code, this scares me a bit ) sometime next week or the following one.
          I also prefer this to be in a separate Jira.

          To clarify the rules some more, let me give you a few examples (this also appears in different levels of details in the javadoc and documentation I added to the code).

          First, in case the current host to which the client is connected is not in the new list
          updateServerList will always cause the connection to be dropped. Otherwise, the decision is based on whether the number of servers has increased or decreased and by how much.

          Suppose that the previous connection string contained 3 hosts and now the list contains these 3 hosts and 2 more hosts, 40% of clients connected to each of the 3 hosts will move to one of the new hosts in order to balance the load. The algorithm will cause the client
          to drop its connection to the current host to which it is connected with probability 0.4 (= 1 - 3/5, rule 1)
          and in this case cause the client to connect to one of the 2 new hosts, chosen at random.

          Another example - suppose we have 5 hosts and now update the list to remove 2 of the hosts, the clients
          connected to the 3 remaining hosts will stay connected (rule 3), whereas all clients connected to the 2 removed hosts
          will need to move to one of the 3 hosts, chosen at random. In this case, the formula in rule 4 simply gives 1 (3(5-3)/(3*2))
          since no new servers were added, so clients connected to the removed hosts have no choice but connecting to the 3 old servers.
          These rules also take into account the case where servers are both added and removed at the same time, and that's why rule
          4 doesn't always give probability 1.

          If the connection is dropped, the client moves to a special mode where he chooses a new server to connect to using the
          probabilistic algorithm, and not jus round robin. In the first example, each client decides to disconnect from the current host with
          probability 0.4 but once the decision is made, it will try to connect to a random NEW server and only if it cannot connect
          to any of the new servers will it try to connect to the old ones.

          After finding a server, or trying all servers in the new list and failing to connect, the client moves back to the normal
          mode of operation where it will pick an arbitrary server from the connectString and attempt to connect to it.

          Show
          Alexander Shraer added a comment - Hi, thanks for the comments!! I removed getCurrentHost, added a JavaDoc for updateServerList, and a paragraph to the documentation. I can do the C client changes (although, after looking at the code, this scares me a bit ) sometime next week or the following one. I also prefer this to be in a separate Jira. To clarify the rules some more, let me give you a few examples (this also appears in different levels of details in the javadoc and documentation I added to the code). First, in case the current host to which the client is connected is not in the new list updateServerList will always cause the connection to be dropped. Otherwise, the decision is based on whether the number of servers has increased or decreased and by how much. Suppose that the previous connection string contained 3 hosts and now the list contains these 3 hosts and 2 more hosts, 40% of clients connected to each of the 3 hosts will move to one of the new hosts in order to balance the load. The algorithm will cause the client to drop its connection to the current host to which it is connected with probability 0.4 (= 1 - 3/5, rule 1) and in this case cause the client to connect to one of the 2 new hosts, chosen at random. Another example - suppose we have 5 hosts and now update the list to remove 2 of the hosts, the clients connected to the 3 remaining hosts will stay connected (rule 3), whereas all clients connected to the 2 removed hosts will need to move to one of the 3 hosts, chosen at random. In this case, the formula in rule 4 simply gives 1 (3(5-3)/(3*2)) since no new servers were added, so clients connected to the removed hosts have no choice but connecting to the 3 old servers. These rules also take into account the case where servers are both added and removed at the same time, and that's why rule 4 doesn't always give probability 1. If the connection is dropped, the client moves to a special mode where he chooses a new server to connect to using the probabilistic algorithm, and not jus round robin. In the first example, each client decides to disconnect from the current host with probability 0.4 but once the decision is made, it will try to connect to a random NEW server and only if it cannot connect to any of the new servers will it try to connect to the old ones. After finding a server, or trying all servers in the new list and failing to connect, the client moves back to the normal mode of operation where it will pick an arbitrary server from the connectString and attempt to connect to it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12512026/ZOOKEEPER-1355-ver6.patch
          against trunk revision 1234974.

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

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

          -1 javadoc. The javadoc tool appears to have generated 8 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/921//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/921//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/921//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/12512026/ZOOKEEPER-1355-ver6.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 8 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/921//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/921//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/921//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -
          Show
          Alexander Shraer added a comment - I believe that this hits https://issues.apache.org/jira/browse/ZOOKEEPER-1357
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12512037/ZOOKEEPER-1355-ver7.patch
          against trunk revision 1234974.

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

          +1 tests included. The patch appears to include 3 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/922//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/922//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/922//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/12512037/ZOOKEEPER-1355-ver7.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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/922//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/922//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/922//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          added a fix for ZOOKEEPER-1357

          Show
          Alexander Shraer added a comment - added a fix for ZOOKEEPER-1357
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12512043/ZOOKEEPER-1355-ver8.patch
          against trunk revision 1234974.

          +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/923//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/923//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/923//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/12512043/ZOOKEEPER-1355-ver8.patch against trunk revision 1234974. +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/923//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/923//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/923//console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          I also prefer this [c client changes] to be in a separate Jira.

          separate jira is fine. but I'd like to see both java and c committed at the same time.

          To clarify the rules some more ....

          I also see you updated the docs which is great. I couldn't tell - is all the salient detail in your comments ("To clarify the rules some more ...") captured in the guide update? Great it's documented here, but in the guide is the more useful going fwd.

          Thanks!

          Show
          Patrick Hunt added a comment - I also prefer this [c client changes] to be in a separate Jira. separate jira is fine. but I'd like to see both java and c committed at the same time. To clarify the rules some more .... I also see you updated the docs which is great. I couldn't tell - is all the salient detail in your comments ("To clarify the rules some more ...") captured in the guide update? Great it's documented here, but in the guide is the more useful going fwd. Thanks!
          Hide
          Alexander Shraer added a comment -

          > I also see you updated the docs which is great. I couldn't tell - is all the salient > detail in your comments ("To clarify the rules some more ...") captured in the guide > update? Great it's documented here, but in the guide is the more useful going fwd.

          Yes, however the pdf in this jira is really what someone would want to read to understand the algorithm. Can we put it somewhere and reference to it from the docs ?

          Show
          Alexander Shraer added a comment - > I also see you updated the docs which is great. I couldn't tell - is all the salient > detail in your comments ("To clarify the rules some more ...") captured in the guide > update? Great it's documented here, but in the guide is the more useful going fwd. Yes, however the pdf in this jira is really what someone would want to read to understand the algorithm. Can we put it somewhere and reference to it from the docs ?
          Hide
          Patrick Hunt added a comment -

          I think having that as a link would be useful. You could attach it on the CWIKI and put a link to that. Given it's a pdf it's going to be hard to make modifications going fwd though. Not sure if that can be helped but if it's an attachment on the wiki we could at least capture notes/changes there.

          Show
          Patrick Hunt added a comment - I think having that as a link would be useful. You could attach it on the CWIKI and put a link to that. Given it's a pdf it's going to be hard to make modifications going fwd though. Not sure if that can be helped but if it's an attachment on the wiki we could at least capture notes/changes there.
          Hide
          Alexander Shraer added a comment -

          ok. thanks!

          Show
          Alexander Shraer added a comment - ok. thanks!
          Hide
          Benjamin Reed added a comment -

          mahadev, pat any more concerns or is this ready to go?

          Show
          Benjamin Reed added a comment - mahadev, pat any more concerns or is this ready to go?
          Hide
          Alexander Shraer added a comment -

          ZOOKEEPER-1355-ver9 applies for zookeeper-3.4.3 and trunk and fixes issues raised on review-board https://reviews.apache.org/r/3781

          Show
          Alexander Shraer added a comment - ZOOKEEPER-1355 -ver9 applies for zookeeper-3.4.3 and trunk and fixes issues raised on review-board https://reviews.apache.org/r/3781
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12516134/ZOOKEEPER-1355-ver9.patch
          against trunk revision 1294000.

          +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 appears to have generated 8 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/962//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/962//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/962//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/12516134/ZOOKEEPER-1355-ver9.patch against trunk revision 1294000. +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 appears to have generated 8 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/962//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/962//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/962//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          small javadoc fix. see comment for ZOOKEEPER-1355-ver9.patch

          Show
          Alexander Shraer added a comment - small javadoc fix. see comment for ZOOKEEPER-1355 -ver9.patch
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12516137/ZOOKEEPER-1355-ver9-1.patch
          against trunk revision 1294000.

          +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/963//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/963//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/963//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/12516137/ZOOKEEPER-1355-ver9-1.patch against trunk revision 1294000. +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/963//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/963//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/963//console This message is automatically generated.
          Hide
          Mark Miller added a comment -

          Looks like this narrowly missed 3.5?

          Show
          Mark Miller added a comment - Looks like this narrowly missed 3.5?
          Hide
          Marshall McMullen added a comment -

          FYI, I've written the C client code to support this feature.

          I've also added a bunch of tests to verify the load balancing aspect of it works correctly. One unit test is currently causing me some issues. But I intent to upload an updated patch later this evening that will include all of Alex's original changes as well as the changes to the C client.

          Show
          Marshall McMullen added a comment - FYI, I've written the C client code to support this feature. I've also added a bunch of tests to verify the load balancing aspect of it works correctly. One unit test is currently causing me some issues. But I intent to upload an updated patch later this evening that will include all of Alex's original changes as well as the changes to the C client.
          Hide
          Marshall McMullen added a comment -

          Uploading a newer version of the patch to include the changes to the C code to support this feature.

          Show
          Marshall McMullen added a comment - Uploading a newer version of the patch to include the changes to the C code to support this feature.
          Hide
          Marshall McMullen added a comment -

          Uploading ZOOKEEPER-1355-ver10.patch.

          This includes all the code that was in ZOOKEEPER-1355-ver9-1.patch as well as all the C code to support this feature.

          Show
          Marshall McMullen added a comment - Uploading ZOOKEEPER-1355 -ver10.patch. This includes all the code that was in ZOOKEEPER-1355 -ver9-1.patch as well as all the C code to support this feature.
          Hide
          Marshall McMullen added a comment -

          Uploading ZOOKEEPER-1355-ver10.patch.

          This includes all the code that was in ZOOKEEPER-1355-ver9-1.patch as well as all the C code to support this feature.

          Show
          Marshall McMullen added a comment - Uploading ZOOKEEPER-1355 -ver10.patch. This includes all the code that was in ZOOKEEPER-1355 -ver9-1.patch as well as all the C code to support this feature.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12518035/ZOOKEEPER-1355-ver10.patch
          against trunk revision 1297740.

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

          +1 tests included. The patch appears to include 31 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/987//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/987//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/987//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/12518035/ZOOKEEPER-1355-ver10.patch against trunk revision 1297740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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/987//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/987//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/987//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Uploading a newer version with a few more minor fixes.

          Show
          Marshall McMullen added a comment - Uploading a newer version with a few more minor fixes.
          Hide
          Marshall McMullen added a comment -

          Newer version has a couple fixes:

          1. Force disconnect if server set reconfig causes the server we're connected to to no longer be in list.

          2. More intelligent timeout if we've tried all the servers so we don't spin.

          Show
          Marshall McMullen added a comment - Newer version has a couple fixes: 1. Force disconnect if server set reconfig causes the server we're connected to to no longer be in list. 2. More intelligent timeout if we've tried all the servers so we don't spin.
          Hide
          Marshall McMullen added a comment -

          Bug fix in C code. Need to update_addrs before checking if *fd == -1 else won't seek new connection after being forcibly disconnected due to server reconfig.

          Show
          Marshall McMullen added a comment - Bug fix in C code. Need to update_addrs before checking if *fd == -1 else won't seek new connection after being forcibly disconnected due to server reconfig.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12518043/ZOOKEEPER-1355-ver10-2.patch
          against trunk revision 1297740.

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

          +1 tests included. The patch appears to include 31 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/988//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/988//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/988//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/12518043/ZOOKEEPER-1355-ver10-2.patch against trunk revision 1297740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/988//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/988//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/988//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          I see my latest change broke existing core C tests.

          I'll upload a new version.

          Show
          Marshall McMullen added a comment - I see my latest change broke existing core C tests. I'll upload a new version.
          Hide
          Marshall McMullen added a comment -

          last change broke the tests. Canceling patch state.

          Show
          Marshall McMullen added a comment - last change broke the tests. Canceling patch state.
          Hide
          Marshall McMullen added a comment -

          Disabling the forced disconnect on server reconfig as it's causing grief on first connect and causing other tests to fail.

          Need to handle first connect more intelligently but don't have time to get to this today.

          Uploading a fixed version so unit tests will pass.

          Will upload an improved version tomorrow that deals with first connection properly.

          Show
          Marshall McMullen added a comment - Disabling the forced disconnect on server reconfig as it's causing grief on first connect and causing other tests to fail. Need to handle first connect more intelligently but don't have time to get to this today. Uploading a fixed version so unit tests will pass. Will upload an improved version tomorrow that deals with first connection properly.
          Hide
          Marshall McMullen added a comment -

          Fixing mismerge on the last patch.

          Show
          Marshall McMullen added a comment - Fixing mismerge on the last patch.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12518053/ZOOKEEPER-1355-ver10-4.patch
          against trunk revision 1297740.

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

          +1 tests included. The patch appears to include 31 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/989//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/989//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/989//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/12518053/ZOOKEEPER-1355-ver10-4.patch against trunk revision 1297740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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/989//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/989//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/989//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Uploading a new version that fixes the C code to properly force a disconnect if the list of servers is modified and the host we were connected to is not in the list anymore.

          Show
          Marshall McMullen added a comment - Uploading a new version that fixes the C code to properly force a disconnect if the list of servers is modified and the host we were connected to is not in the list anymore.
          Hide
          Marshall McMullen added a comment -

          Current C tests all pass except for a couple that wait for disconnected state. Didn't have time to sort these out today. Will try to get to that this evening or tomorrow, then I'll upload a new patch.

          Will also be adding some more rigorous tests now that I know more about how I can test this more thoroughly.

          Show
          Marshall McMullen added a comment - Current C tests all pass except for a couple that wait for disconnected state. Didn't have time to sort these out today. Will try to get to that this evening or tomorrow, then I'll upload a new patch. Will also be adding some more rigorous tests now that I know more about how I can test this more thoroughly.
          Hide
          Marshall McMullen added a comment -

          This version fixes the core unit tests that were failing.

          All tests pass locally for me now.

          Show
          Marshall McMullen added a comment - This version fixes the core unit tests that were failing. All tests pass locally for me now.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520208/ZOOKEEPER-1355-ver11.patch
          against trunk revision 1302736.

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

          +1 tests included. The patch appears to include 34 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1016//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1016//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1016//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/12520208/ZOOKEEPER-1355-ver11.patch against trunk revision 1302736. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1016//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1016//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1016//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Alex, looks like only one unit test failure – on the Java unit test side. So it can't be related to the C changes I made.

          Can you take a look when you get time?

          Also, no one has reviewed my proposed C changes so would be good to start getting some eyes on it.

          Note I added a new addrvec (address vector) data structure to make it easier to manage and manipulate the 3 different list of addresses (current, joining, leaving).

          Show
          Marshall McMullen added a comment - Alex, looks like only one unit test failure – on the Java unit test side. So it can't be related to the C changes I made. Can you take a look when you get time? Also, no one has reviewed my proposed C changes so would be good to start getting some eyes on it. Note I added a new addrvec (address vector) data structure to make it easier to manage and manipulate the 3 different list of addresses (current, joining, leaving).
          Hide
          Alexander Shraer added a comment -

          Thanks Marshall! the failing test seems to be the one addressed in ZOOKEEPER-1433.

          Show
          Alexander Shraer added a comment - Thanks Marshall! the failing test seems to be the one addressed in ZOOKEEPER-1433 .
          Hide
          Marshall McMullen added a comment -

          Minor bugfix in count_hosts in case an empty string is passed in.

          Show
          Marshall McMullen added a comment - Minor bugfix in count_hosts in case an empty string is passed in.
          Hide
          Marshall McMullen added a comment -

          Minor bug fix for count_hosts in case an empty string is passed in.

          Show
          Marshall McMullen added a comment - Minor bug fix for count_hosts in case an empty string is passed in.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520766/ZOOKEEPER-1355-ver11-1.patch
          against trunk revision 1307644.

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

          +1 tests included. The patch appears to include 34 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/1025//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1025//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1025//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/12520766/ZOOKEEPER-1355-ver11-1.patch against trunk revision 1307644. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 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/1025//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1025//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1025//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          I posted this latest patch on review board: https://reviews.apache.org/r/4596/

          Show
          Marshall McMullen added a comment - I posted this latest patch on review board: https://reviews.apache.org/r/4596/
          Hide
          Flavio Junqueira added a comment -

          I'm reviewing this one.

          Show
          Flavio Junqueira added a comment - I'm reviewing this one.
          Hide
          Flavio Junqueira added a comment -

          There are currently two separate reviews on the review board for this jira, one by Alex and one by Marshall. It would be great if we could keep one review request and update it with new revisions. And, just so that the comments from review board are recorded in the jira, please make sure to update the bugs field with the jira number.

          I have also already reviewed the java part of this patch, but I don't see any of my previous comments reflected.

          Could you guys please fix all these?

          Show
          Flavio Junqueira added a comment - There are currently two separate reviews on the review board for this jira, one by Alex and one by Marshall. It would be great if we could keep one review request and update it with new revisions. And, just so that the comments from review board are recorded in the jira, please make sure to update the bugs field with the jira number. I have also already reviewed the java part of this patch, but I don't see any of my previous comments reflected. Could you guys please fix all these?
          Hide
          Alexander Shraer added a comment -

          Done. I updated the new patch in https://reviews.apache.org/r/3781/ and answered the previous comments

          Show
          Alexander Shraer added a comment - Done. I updated the new patch in https://reviews.apache.org/r/3781/ and answered the previous comments
          Hide
          Marshall McMullen added a comment -

          Updated C code only to fix some unsafe locking around the server list and to properly throttle reconnect logic when we've tried all the servers in the server list unsuccessfully.

          Show
          Marshall McMullen added a comment - Updated C code only to fix some unsafe locking around the server list and to properly throttle reconnect logic when we've tried all the servers in the server list unsuccessfully.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12527533/ZOOKEEPER-1355-ver12.patch
          against trunk revision 1337029.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1073//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/12527533/ZOOKEEPER-1355-ver12.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1073//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Changes to trunk since this patch was originally built.

          Show
          Marshall McMullen added a comment - Changes to trunk since this patch was originally built.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12527535/ZOOKEEPER-1355-ver12-1.patch
          against trunk revision 1337029.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1074//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/12527535/ZOOKEEPER-1355-ver12-1.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1074//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          This one should apply to tip of trunk cleanly.

          Show
          Marshall McMullen added a comment - This one should apply to tip of trunk cleanly.
          Hide
          Marshall McMullen added a comment -

          This one should apply cleanly to tip of trunk.

          Show
          Marshall McMullen added a comment - This one should apply cleanly to tip of 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/12527539/ZOOKEEPER-1355-ver12-2.patch
          against trunk revision 1337029.

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

          +1 tests included. The patch appears to include 34 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1075//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1075//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1075//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/12527539/ZOOKEEPER-1355-ver12-2.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1075//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1075//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1075//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Updated to work correctly against tip of trunk. All unit tests should pass now.

          Show
          Marshall McMullen added a comment - Updated to work correctly against tip of trunk. All unit tests should pass now.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12527735/ZOOKEEPER-1355-ver12-4.patch
          against trunk revision 1337029.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1077//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/12527735/ZOOKEEPER-1355-ver12-4.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1077//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Bad patch last time, my apologies.

          Bumped the version on this one to 13 to avoid confusion.

          Show
          Marshall McMullen added a comment - Bad patch last time, my apologies. Bumped the version on this one to 13 to avoid confusion.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12527742/ZOOKEEPER-1355-ver13.patch
          against trunk revision 1337029.

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

          +1 tests included. The patch appears to include 34 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/1078//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1078//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1078//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/12527742/ZOOKEEPER-1355-ver13.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 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/1078//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1078//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1078//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Typo in src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml: "jus" should be "just".

          The documentation is good - perhaps it could make reference in the source code to where the client's server-selection logic is implemented (StaticHostProvider::updateServerList()).

          i.e. provide a link to the source such as http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java?view=markup or https://github.com/apache/zookeeper/blob/trunk/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java

          The test cases seem to have a lot more cases than the documentation: would be nice to have the doc's examples correspond to the test cases.

          Show
          Eugene Koontz added a comment - Typo in src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml: "jus" should be "just". The documentation is good - perhaps it could make reference in the source code to where the client's server-selection logic is implemented (StaticHostProvider::updateServerList()). i.e. provide a link to the source such as http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java?view=markup or https://github.com/apache/zookeeper/blob/trunk/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java The test cases seem to have a lot more cases than the documentation: would be nice to have the doc's examples correspond to the test cases.
          Hide
          Marshall McMullen added a comment -

          This updated patch fixes 3 things in the C version only:

          (1) Fix memleak in update_addrs in the event that the addrvec hasn't changed from last time.

          (2) Fix unsafe error handling around realloc failure in addrvec.c.

          (3) Fix connection failure throttling that caused it to skip a host.

          Show
          Marshall McMullen added a comment - This updated patch fixes 3 things in the C version only: (1) Fix memleak in update_addrs in the event that the addrvec hasn't changed from last time. (2) Fix unsafe error handling around realloc failure in addrvec.c. (3) Fix connection failure throttling that caused it to skip a host.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12531858/ZOOKEEPER-1355-ver14.patch
          against trunk revision 1337029.

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

          +1 tests included. The patch appears to include 34 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/1094//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1094//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1094//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/12531858/ZOOKEEPER-1355-ver14.patch against trunk revision 1337029. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 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/1094//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1094//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1094//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12548653/ZOOKEEPER-1355-10-Oct.patch
          against trunk revision 1391526.

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

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

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

          -1 javac. The patch appears to cause tar ant target to fail.

          +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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1209//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1209//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1209//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/12548653/ZOOKEEPER-1355-10-Oct.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1209//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1209//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1209//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12548688/ZOOKEEPER-1355-10-Oct.patch
          against trunk revision 1391526.

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

          +1 tests included. The patch appears to include 28 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1210//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1210//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1210//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/12548688/ZOOKEEPER-1355-10-Oct.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 28 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1210//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1210//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1210//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          This is an updated version of patch that applies cleanly to the latest tip of trunk.

          Show
          Marshall McMullen added a comment - This is an updated version of patch that applies cleanly to the latest tip of 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/12548996/ZOOKEEPER-1355-12-Oct.patch
          against trunk revision 1391526.

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

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

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

          -1 javac. The patch appears to cause tar ant target to fail.

          +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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1221//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1221//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1221//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/12548996/ZOOKEEPER-1355-12-Oct.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 34 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1221//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1221//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1221//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Had meant to remove the Zab Test part from this patch as Alex tells me that was already committed to trunk under another Jira.

          Show
          Marshall McMullen added a comment - Had meant to remove the Zab Test part from this patch as Alex tells me that was already committed to trunk under another 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/12549007/ZOOKEEPER-1355-13-Oct.patch
          against trunk revision 1391526.

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

          +1 tests included. The patch appears to include 31 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/1222//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1222//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1222//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/12549007/ZOOKEEPER-1355-13-Oct.patch against trunk revision 1391526. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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/1222//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1222//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1222//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Addresses all the concerns Michi had with the C client over on review board https://reviews.apache.org/r/3781/#review12460

          Show
          Marshall McMullen added a comment - Addresses all the concerns Michi had with the C client over on review board https://reviews.apache.org/r/3781/#review12460
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552299/ZOOKEEPER-1355-6-Nov.patch
          against trunk revision 1404288.

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

          +1 tests included. The patch appears to include 31 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1250//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1250//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1250//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/12552299/ZOOKEEPER-1355-6-Nov.patch against trunk revision 1404288. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1250//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1250//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1250//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Removal of srand48 seems to have broken one of the new reconfig tests. Adding it back in.

          Show
          Marshall McMullen added a comment - Removal of srand48 seems to have broken one of the new reconfig tests. Adding it back in.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552304/ZOOKEEPER-1355-6-Nov-ver2.patch
          against trunk revision 1404288.

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

          +1 tests included. The patch appears to include 31 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/1251//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1251//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1251//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/12552304/ZOOKEEPER-1355-6-Nov-ver2.patch against trunk revision 1404288. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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/1251//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1251//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1251//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          michi and flavio are you guys good with this? please express concern or +1 if i don't hear anything by tomorrow, i'll assume the latter.

          Show
          Benjamin Reed added a comment - michi and flavio are you guys good with this? please express concern or +1 if i don't hear anything by tomorrow, i'll assume the latter.
          Hide
          Michi Mutsuzaki added a comment -

          +1 for the C client part.

          --Michi

          Show
          Michi Mutsuzaki added a comment - +1 for the C client part. --Michi
          Hide
          Flavio Junqueira added a comment -

          Thanks Marshall and Alex for working on the patch. Is the latest patch on the review board? Until I check it on the review board it is -1 for me.

          Show
          Flavio Junqueira added a comment - Thanks Marshall and Alex for working on the patch. Is the latest patch on the review board? Until I check it on the review board it is -1 for me.
          Hide
          Marshall McMullen added a comment -

          Flavio, the newest patch is not on review board as I couldn't figure out how to update review board with the latest patch... :-<

          Show
          Marshall McMullen added a comment - Flavio, the newest patch is not on review board as I couldn't figure out how to update review board with the latest patch... :-<
          Hide
          Alexander Shraer added a comment -

          Hi Flavio,

          I uploaded Marshall's latest patch to review board: https://reviews.apache.org/r/3781/

          Alex

          Show
          Alexander Shraer added a comment - Hi Flavio, I uploaded Marshall's latest patch to review board: https://reviews.apache.org/r/3781/ Alex
          Hide
          Marshall McMullen added a comment -

          Updated patch that addresses all comments in review https://reviews.apache.org/r/3781/

          Show
          Marshall McMullen added a comment - Updated patch that addresses all comments in review https://reviews.apache.org/r/3781/
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12553459/ZOOKEEPER-1355-13-Nov.patch
          against trunk revision 1404288.

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

          +1 tests included. The patch appears to include 31 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 failed core unit tests.

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1260//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1260//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1260//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/12553459/ZOOKEEPER-1355-13-Nov.patch against trunk revision 1404288. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1260//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1260//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1260//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Fix compile failure after refactor based on code review.

          Show
          Marshall McMullen added a comment - Fix compile failure after refactor based on code review.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12553464/ZOOKEEPER-1355-13-Nov-ver2.patch
          against trunk revision 1404288.

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

          +1 tests included. The patch appears to include 31 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/1261//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1261//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1261//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/12553464/ZOOKEEPER-1355-13-Nov-ver2.patch against trunk revision 1404288. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 31 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/1261//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1261//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1261//console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          +1, thanks guys for the work. Is anyone else interested in reviewing this patch? If so, please comment asap. I'm fine with committing it myself before the end of the week or having someone else doing it, like Ben, since he pushed a lot for this patch.

          Show
          Flavio Junqueira added a comment - +1, thanks guys for the work. Is anyone else interested in reviewing this patch? If so, please comment asap. I'm fine with committing it myself before the end of the week or having someone else doing it, like Ben, since he pushed a lot for this patch.
          Hide
          Patrick Hunt added a comment -

          Seems reasonable to me. (cursory look). As long as it doesn't effect backward compatibility I don't see an issue.

          Show
          Patrick Hunt added a comment - Seems reasonable to me. (cursory look). As long as it doesn't effect backward compatibility I don't see an issue.
          Hide
          Alexander Shraer added a comment -

          I think that it doesn't affect backward compatibility. The code works as before unless updateServerList is actually invoked. When it does, it updates the list of servers with a new shuffled list and then uses new logic to choose the first server to connect to from this list. If it succeeds to connect or tried all servers and failed to connect, we return to the "normal mode" of just going round-robin over the new shuffled list. No configuration files are touched here and nothing on the server side changed.

          Show
          Alexander Shraer added a comment - I think that it doesn't affect backward compatibility. The code works as before unless updateServerList is actually invoked. When it does, it updates the list of servers with a new shuffled list and then uses new logic to choose the first server to connect to from this list. If it succeeds to connect or tried all servers and failed to connect, we return to the "normal mode" of just going round-robin over the new shuffled list. No configuration files are touched here and nothing on the server side changed.
          Hide
          Flavio Junqueira added a comment -

          Good job, guys. Committed revision 1410731.

          Show
          Flavio Junqueira added a comment - Good job, guys. Committed revision 1410731.

            People

            • Assignee:
              Alexander Shraer
              Reporter:
              Alexander Shraer
            • Votes:
              3 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development