ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1633

Introduce a protocol version to connection initiation message

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.6
    • Component/s: server
    • Labels:
      None

      Description

      Currently the first message a server sends to another server includes just one field - the server's id (long). This is in QuorumCnxManager.java. This makes changes to the information passed during this initial connection very difficult. This patch will change the first field of the message to be a protocol version (a negative number that can't be a server id). The second field will be the server id. The third field is number of bytes in the remainder of the message. A 3.4 server will read the first field as before, but if this is a negative number it will read the second field to find the server id, and then remove the remainder of the message from the stream. This will not affect 3.4 since 3.4 and earlier servers send just the server id (so the code in the patch will not run unless there is a server > 3.4 trying to connect). This will, however, provide the necessary flexibility for future releases as well as an upgrade path from 3.4

      1. ZOOKEEPER-1633-ver3.patch
        6 kB
        Alexander Shraer
      2. ZOOKEEPER-1633-ver2.patch
        6 kB
        Alexander Shraer
      3. ZOOKEEPER-1633-v4.patch
        20 kB
        Flavio Junqueira
      4. ZOOKEEPER-1633-v4.patch
        33 kB
        Flavio Junqueira
      5. ZOOKEEPER-1633.patch
        1 kB
        Alexander Shraer

        Issue Links

          Activity

          Hide
          Flavio Junqueira added a comment -

          Closing issues after releasing 3.4.6.

          Show
          Flavio Junqueira added a comment - Closing issues after releasing 3.4.6.
          Hide
          Raul Gutierrez Segales added a comment -

          Now that I come to think of it, I might have seen it in prod.

          Show
          Raul Gutierrez Segales added a comment - Now that I come to think of it, I might have seen it in prod.
          Hide
          Alexander Shraer added a comment -

          I'm not sure if ObserverId = -1 is actually something we need to support ? Is it used by anyone ?
          Perhaps someone can clarify Benjamin Reed Henry Robinson Flavio Junqueira ?

          In any case, some of the reconfig functionality such as converting observers to followers is not supported for this kind of
          observers.

          Show
          Alexander Shraer added a comment - I'm not sure if ObserverId = -1 is actually something we need to support ? Is it used by anyone ? Perhaps someone can clarify Benjamin Reed Henry Robinson Flavio Junqueira ? In any case, some of the reconfig functionality such as converting observers to followers is not supported for this kind of observers.
          Hide
          Raul Gutierrez Segales added a comment -

          Ah - never mind. Alex clarified this in ZOOKEEPER-1789.

          Show
          Raul Gutierrez Segales added a comment - Ah - never mind. Alex clarified this in ZOOKEEPER-1789 .
          Hide
          Raul Gutierrez Segales added a comment -

          Are observers mandated to have -1 as their sid? (lots of test cases in the code base (and deployments!) use something > 0). Plus the docs don't indicate that -1 should be used: http://zookeeper.apache.org/doc/r3.3.1/zookeeperObservers.html.

          Show
          Raul Gutierrez Segales added a comment - Are observers mandated to have -1 as their sid? (lots of test cases in the code base (and deployments!) use something > 0). Plus the docs don't indicate that -1 should be used: http://zookeeper.apache.org/doc/r3.3.1/zookeeperObservers.html .
          Hide
          Alexander Shraer added a comment -

          Seems like there is a small bug in the patch. Instead of "if (sid < 0) {" it should say "if (sid < -1)" because of the special ObserverId which is -1.

          Show
          Alexander Shraer added a comment - Seems like there is a small bug in the patch. Instead of "if (sid < 0) {" it should say "if (sid < -1)" because of the special ObserverId which is -1.
          Hide
          Flavio Junqueira added a comment -

          Revision number r1463399. http://svn.apache.org/r1463399

          Show
          Flavio Junqueira added a comment - Revision number r1463399. http://svn.apache.org/r1463399
          Hide
          Alexander Shraer added a comment -

          Hi Ted,

          Thanks for the comments. In this specific patch I was trying to solve the following problem: currently the first thing a server
          wants to hear from another server connecting to it is a long, which is the id of the connecting server. Once the connection is
          established, and a leader is elected, they start sending protobufs. For ZK-107 an id was not enough - I also needed to send the leader election address and port (a string) of the connecting server, which may be new. We needed a way for an old 3.4 server
          to understand that the connecting server is sending id + string/protobuf and not just id, and at least remove the string/protobuf from the stream, even if it only ends up using the id from this message. This is why in the latter case we're sending a negative number followed by an id, followed by the length of the remaining message, and the 3.4 server can realize that this is not just "id", remove the remaining message from the stream, and move on.

          So while I agree with you that it would be nicer if that first message is a protobuf and not just a long as in 3.4, I don't see how this would help us solve the backward compatibility issue that this current patch is trying to address.

          Thanks,
          Alex

          Show
          Alexander Shraer added a comment - Hi Ted, Thanks for the comments. In this specific patch I was trying to solve the following problem: currently the first thing a server wants to hear from another server connecting to it is a long, which is the id of the connecting server. Once the connection is established, and a leader is elected, they start sending protobufs. For ZK-107 an id was not enough - I also needed to send the leader election address and port (a string) of the connecting server, which may be new. We needed a way for an old 3.4 server to understand that the connecting server is sending id + string/protobuf and not just id, and at least remove the string/protobuf from the stream, even if it only ends up using the id from this message. This is why in the latter case we're sending a negative number followed by an id, followed by the length of the remaining message, and the 3.4 server can realize that this is not just "id", remove the remaining message from the stream, and move on. So while I agree with you that it would be nicer if that first message is a protobuf and not just a long as in 3.4, I don't see how this would help us solve the backward compatibility issue that this current patch is trying to address. Thanks, Alex
          Hide
          Ted Dunning added a comment -

          Alex,

          Sorry to come in late here.

          I have a few comments.

          1) is a version number really what you want here? Shouldn't it be done more like modern protocols such as protobufs to introduce a mechanism of optional fields? Strict versioning of protocols is very unpopular any more because of the brittleness introduced into protocols.

          2) it is possible that there is an irreconciliable conflict in version between correspondents. In such cases, it is important to signal this clearly. As such, it is good to add not only versioning information to the original request, but a very stable reply that indicates that there is an irreconciliable version mismatch. Is there a way that you can do this in your proposal?

          Show
          Ted Dunning added a comment - Alex, Sorry to come in late here. I have a few comments. 1) is a version number really what you want here? Shouldn't it be done more like modern protocols such as protobufs to introduce a mechanism of optional fields? Strict versioning of protocols is very unpopular any more because of the brittleness introduced into protocols. 2) it is possible that there is an irreconciliable conflict in version between correspondents. In such cases, it is important to signal this clearly. As such, it is good to add not only versioning information to the original request, but a very stable reply that indicates that there is an irreconciliable version mismatch. Is there a way that you can do this in your proposal?
          Hide
          Alexander Shraer added a comment -

          looks like this can be committed

          Show
          Alexander Shraer added a comment - looks like this can be committed
          Hide
          Flavio Junqueira added a comment -

          +1 for me too. does anyone else want to comment on this patch?

          Show
          Flavio Junqueira added a comment - +1 for me too. does anyone else want to comment on this patch?
          Hide
          Alexander Shraer added a comment -

          looks fine to me. thanks!

          Show
          Alexander Shraer added a comment - looks fine to me. thanks!
          Hide
          Flavio Junqueira added a comment -

          Ok, fixed. Could you check it out now, Alex?

          Show
          Flavio Junqueira added a comment - Ok, fixed. Could you check it out now, Alex?
          Hide
          Flavio Junqueira added a comment -

          yeah, so here is what svn status shows me:

          D       src/java/test/org/apache/zookeeper/test/CnxManagerTest.java
          A  +    src/java/test/org/apache/zookeeper/server/quorum/CnxManagerTest.java
          M       src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
          

          that A + apparently means that the file is moved upon commit. I thought that svn knew what it was doing but it probably doesn't. let me try to figure out if it is possible to remove the +.

          Show
          Flavio Junqueira added a comment - yeah, so here is what svn status shows me: D src/java/test/org/apache/zookeeper/test/CnxManagerTest.java A + src/java/test/org/apache/zookeeper/server/quorum/CnxManagerTest.java M src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java that A + apparently means that the file is moved upon commit. I thought that svn knew what it was doing but it probably doesn't. let me try to figure out if it is possible to remove the +.
          Hide
          Alexander Shraer added a comment -

          good idea about changing the package. did you do svn add for the new file ? (the patch doesn't apply for me)

          Show
          Alexander Shraer added a comment - good idea about changing the package. did you do svn add for the new file ? (the patch doesn't apply for me)
          Hide
          Flavio Junqueira added a comment -

          Alexander Shraer I have implemented what I was suggesting and I have moved the test file to a different package to make access easier. Let me know if it works for you.

          Show
          Flavio Junqueira added a comment - Alexander Shraer I have implemented what I was suggesting and I have moved the test file to a different package to make access easier. Let me know if it works for you.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12571678/ZOOKEEPER-1633-ver3.patch
          against trunk revision 1448007.

          +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/1412//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1412//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1412//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/12571678/ZOOKEEPER-1633-ver3.patch against trunk revision 1448007. +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/1412//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1412//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1412//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          Flavio,

          First, I didn't find the mock class you're talking about. In any case, if I follow what you're saying, and we extend QCM, in order
          to access senderWorkerMap we have to make it protected or public (it is currently private). Then why go to all this trouble and not
          just make it public and use it directly from the test ? I already had to change a few other fields to public for this same test. I remind you
          that the test and all of the changes in this jira are not going to the main branch - they will only be in 3.4.

          Alex

          Show
          Alexander Shraer added a comment - Flavio, First, I didn't find the mock class you're talking about. In any case, if I follow what you're saying, and we extend QCM, in order to access senderWorkerMap we have to make it protected or public (it is currently private). Then why go to all this trouble and not just make it public and use it directly from the test ? I already had to change a few other fields to public for this same test. I remind you that the test and all of the changes in this jira are not going to the main branch - they will only be in 3.4. Alex
          Hide
          Flavio Junqueira added a comment -

          The test case looks mostly good, Alex. I just have one request. I'd rather have a mock cnxmanager class extending QCM and adding the method you currently have in QCM:

          +    // used from a test to know that a connection succeeded
          +    public boolean testServerInServerWorkerMap(long sid) {
          +    	return senderWorkerMap.containsKey(sid);
          +    }
          +
          

          I thought we had a mock class already in CnxManageTest. If I'm not mistaken, perhaps you could add the method you need there and instantiate that class instead of adding this test method to QCM directly. Does it make sense?

          Show
          Flavio Junqueira added a comment - The test case looks mostly good, Alex. I just have one request. I'd rather have a mock cnxmanager class extending QCM and adding the method you currently have in QCM: + // used from a test to know that a connection succeeded + public boolean testServerInServerWorkerMap(long sid) { + return senderWorkerMap.containsKey(sid); + } + I thought we had a mock class already in CnxManageTest. If I'm not mistaken, perhaps you could add the method you need there and instantiate that class instead of adding this test method to QCM directly. Does it make sense?
          Hide
          Hadoop QA added a comment -

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

          +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/1411//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1411//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1411//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/12571138/ZOOKEEPER-1633-ver2.patch against trunk revision 1448007. +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/1411//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1411//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1411//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          Attached patch includes a test. I verified that the test fails without the patch code. Flavio Junqueira can you please take a look and commit if everything's fine ?

          Show
          Alexander Shraer added a comment - Attached patch includes a test. I verified that the test fails without the patch code. Flavio Junqueira can you please take a look and commit if everything's fine ?
          Hide
          Alexander Shraer added a comment -

          what I'm trying to say is that a unit test like this will test something very limited (that the very beginning of an upgrade process works). I don't mind adding it, just think this is better tested by the separate upgrade testing that's done before releases.

          Show
          Alexander Shraer added a comment - what I'm trying to say is that a unit test like this will test something very limited (that the very beginning of an upgrade process works). I don't mind adding it, just think this is better tested by the separate upgrade testing that's done before releases.
          Hide
          Alexander Shraer added a comment -

          sure, I can add this, but just to clarify - this is a 3.4.6 only code (that will only run during upgrade to 3.5.0). The test is 3.4.6 only too. I can add a test that will just make sure that the receiving server finds the connecting server's id in the stream and ignores the rest. If you were thinking of something different please let me know.

          Show
          Alexander Shraer added a comment - sure, I can add this, but just to clarify - this is a 3.4.6 only code (that will only run during upgrade to 3.5.0). The test is 3.4.6 only too. I can add a test that will just make sure that the receiving server finds the connecting server's id in the stream and ignores the rest. If you were thinking of something different please let me know.
          Hide
          Flavio Junqueira added a comment -

          The intention is for this patch to be in 3.4.6. It will have no effect on 3.4.5 and 3.4.6 normal operation, since they will not send a protocol number, they'll send their id

          agreed.

          Starting with 3.5.0, we'll send the protocol number first, and this patch will allow a 3.5.0 server to connect to a 3.4.6 server. Of course if a 3.5.0 server tries to connect to a 3.4.5 server (that doesn't have the patch) we'll get the error message above (so if you get the errors above this means you haven't applied the patch, Patrick asked me to document this).

          I just noticed that my example was not correct, but I realize now (duh!) that 3.4.6 servers won't send protocol numbers. They are only capable of receiving them.

          about a test case, I was referring to the testing the upgrade process by testing that the interaction with quorum cnx manager will work as expected once we send the protocol number.

          Show
          Flavio Junqueira added a comment - The intention is for this patch to be in 3.4.6. It will have no effect on 3.4.5 and 3.4.6 normal operation, since they will not send a protocol number, they'll send their id agreed. Starting with 3.5.0, we'll send the protocol number first, and this patch will allow a 3.5.0 server to connect to a 3.4.6 server. Of course if a 3.5.0 server tries to connect to a 3.4.5 server (that doesn't have the patch) we'll get the error message above (so if you get the errors above this means you haven't applied the patch, Patrick asked me to document this). I just noticed that my example was not correct, but I realize now (duh!) that 3.4.6 servers won't send protocol numbers. They are only capable of receiving them. about a test case, I was referring to the testing the upgrade process by testing that the interaction with quorum cnx manager will work as expected once we send the protocol number.
          Hide
          Thawan Kooburat added a comment -

          I think the release note section for ZK-107 will have to mention that rolling upgrade to 3.5.x is only possible via 3.4.6 (or any release that have this patch)

          Show
          Thawan Kooburat added a comment - I think the release note section for ZK-107 will have to mention that rolling upgrade to 3.5.x is only possible via 3.4.6 (or any release that have this patch)
          Hide
          Alexander Shraer added a comment -

          The intention is for this patch to be in 3.4.6. It will have no effect on 3.4.5 and 3.4.6 normal operation, since they will not send
          a protocol number, they'll send their id. Starting with 3.5.0, we'll send the protocol number first, and this patch will allow a 3.5.0 server to connect to a 3.4.6 server. Of course if a 3.5.0 server tries to connect to a 3.4.5 server (that doesn't have the patch) we'll get the error message above (so if you get the errors above this means you haven't applied the patch, Patrick asked me to document this).

          My intention was to start sending the protocol version starting with 3.5.0 in future releases too and this is part of zk-107 that already does it.

          In my view this has absolutely no effect on 3.4 since the code will never run until a 3.5.0 or later server tries to connect (the if in the patch will be false since before 3.5.0 servers send their id first, which is positive).

          I tested this with servers running trunk + zk-107 connecting to servers running 3.4 + this patch.

          Show
          Alexander Shraer added a comment - The intention is for this patch to be in 3.4.6. It will have no effect on 3.4.5 and 3.4.6 normal operation, since they will not send a protocol number, they'll send their id. Starting with 3.5.0, we'll send the protocol number first, and this patch will allow a 3.5.0 server to connect to a 3.4.6 server. Of course if a 3.5.0 server tries to connect to a 3.4.5 server (that doesn't have the patch) we'll get the error message above (so if you get the errors above this means you haven't applied the patch, Patrick asked me to document this). My intention was to start sending the protocol version starting with 3.5.0 in future releases too and this is part of zk-107 that already does it. In my view this has absolutely no effect on 3.4 since the code will never run until a 3.5.0 or later server tries to connect (the if in the patch will be false since before 3.5.0 servers send their id first, which is positive). I tested this with servers running trunk + zk-107 connecting to servers running 3.4 + this patch.
          Hide
          Flavio Junqueira added a comment -

          Alexander Shraer the idea of adding a protocol version sounds like a good one, but I'm wondering how this patch is going to work out. If a 3.5.0 server tries to connect to a 3.4.5 server, then I believe it will be dropping the connection according to the log messages above. I forgot exactly how it goes, but I think the one with highest id is supposed to start the connection. If that's the case, then the two servers will never be able to establish a connection.

          About the protocol version, I suppose it wouldn't hurt to have it in future versions too, it is a good practice and we should have done it before. It might be a good idea to create a jira for it if you don't want to this feature more generally.

          I was also wondering if we need a test here. Since this is just to enable migrations from 3.4.5 to 3.5.0, it is not necessary to guarantee no regression in future 3.5.0, but we might need to make sure that the 3.4 remains correct. What do you think?

          Show
          Flavio Junqueira added a comment - Alexander Shraer the idea of adding a protocol version sounds like a good one, but I'm wondering how this patch is going to work out. If a 3.5.0 server tries to connect to a 3.4.5 server, then I believe it will be dropping the connection according to the log messages above. I forgot exactly how it goes, but I think the one with highest id is supposed to start the connection. If that's the case, then the two servers will never be able to establish a connection. About the protocol version, I suppose it wouldn't hurt to have it in future versions too, it is a good practice and we should have done it before. It might be a good idea to create a jira for it if you don't want to this feature more generally. I was also wondering if we need a test here. Since this is just to enable migrations from 3.4.5 to 3.5.0, it is not necessary to guarantee no regression in future 3.5.0, but we might need to make sure that the 3.4 remains correct. What do you think?
          Hide
          Alexander Shraer added a comment -

          Just to clarify - this patch enables upgrade from 3.4 to 3.5, but the upgrade has to go through 3.4.6.
          If a 3.5.0 server attempts to connect to a 3.4.5 server, this is the error that will be produced (the negative server id is in fact the protocol number which is sent first by 3.5.0):

          2013-01-30 11:32:10,663 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60876
          2013-01-30 11:32:10,663 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536
          2013-01-30 11:32:11,466 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60878
          2013-01-30 11:32:11,467 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536
          2013-01-30 11:32:13,069 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60880
          2013-01-30 11:32:13,070 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536
          2013-01-30 11:32:16,273 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60882
          2013-01-30 11:32:16,273 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536

          Show
          Alexander Shraer added a comment - Just to clarify - this patch enables upgrade from 3.4 to 3.5, but the upgrade has to go through 3.4.6. If a 3.5.0 server attempts to connect to a 3.4.5 server, this is the error that will be produced (the negative server id is in fact the protocol number which is sent first by 3.5.0): 2013-01-30 11:32:10,663 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60876 2013-01-30 11:32:10,663 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536 2013-01-30 11:32:11,466 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60878 2013-01-30 11:32:11,467 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536 2013-01-30 11:32:13,069 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60880 2013-01-30 11:32:13,070 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536 2013-01-30 11:32:16,273 [myid:2] - INFO [localhost/127.0.0.1:2784:QuorumCnxManager$Listener@498] - Received connection request /127.0.0.1:60882 2013-01-30 11:32:16,273 [myid:2] - WARN [localhost/127.0.0.1:2784:QuorumCnxManager@349] - Invalid server id: -65536
          Hide
          Flavio Junqueira added a comment -

          Ok, apparently I was wrong and this jira shouldn't go to 3.5.0, only to the 3.4 branch.

          Show
          Flavio Junqueira added a comment - Ok, apparently I was wrong and this jira shouldn't go to 3.5.0, only to the 3.4 branch.
          Hide
          Hadoop QA added a comment -

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

          +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 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 generated 26 release audit warnings (more than the trunk's current 24 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/1372//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1372//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1372//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1372//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/12567326/ZOOKEEPER-1633.patch against trunk revision 1438375. +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 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 generated 26 release audit warnings (more than the trunk's current 24 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/1372//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1372//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1372//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1372//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/12567320/ZOOKEEPER-1633.patch
          against trunk revision 1438375.

          +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 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 generated 26 release audit warnings (more than the trunk's current 24 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/1371//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1371//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1371//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1371//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/12567320/ZOOKEEPER-1633.patch against trunk revision 1438375. +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 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 generated 26 release audit warnings (more than the trunk's current 24 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/1371//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1371//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1371//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1371//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          Attaching patch from 3.4 branch.

          Show
          Alexander Shraer added a comment - Attaching patch from 3.4 branch.
          Hide
          Alexander Shraer added a comment -

          This is required by ZK-107 in order for newly joining servers to send their leader election address during initial connection (since they are new no one has their address).

          During upgrade from 3.4, the 3.4 doesn't actually need this information since membership doesn't change. But without the patch the 3.4 server will not be able to find the server id in the message or remove the entire message from the stream.

          Show
          Alexander Shraer added a comment - This is required by ZK-107 in order for newly joining servers to send their leader election address during initial connection (since they are new no one has their address). During upgrade from 3.4, the 3.4 doesn't actually need this information since membership doesn't change. But without the patch the 3.4 server will not be able to find the server id in the message or remove the entire message from the stream.

            People

            • Assignee:
              Alexander Shraer
              Reporter:
              Alexander Shraer
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development