ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1807

Observers spam each other creating connections to the election addr

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Blocker Blocker
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.5.2, 3.6.0
    • Component/s: None
    • Labels:
      None

      Description

      Hey Alexander Shraer,

      I noticed today that my Observers are spamming each other trying to open connections to the election port. I've got tons of these:

      2013-11-01 22:19:45,819 - DEBUG [WorkerSender[myid=13]] - There is a connection already for server 9
      2013-11-01 22:19:45,819 - DEBUG [WorkerSender[myid=13]] - There is a connection already for server 10
      2013-11-01 22:19:45,819 - DEBUG [WorkerSender[myid=13]] - There is a connection already for server 6
      2013-11-01 22:19:45,819 - DEBUG [WorkerSender[myid=13]] - There is a connection already for server 12
      2013-11-01 22:19:45,819 - DEBUG [WorkerSender[myid=13]] - There is a connection already for server 14
      

      and so and so on ad nauseam.

      Now, looking around I found this inside FastLeaderElection.java from when you committed ZOOKEEPER-107:

           private void sendNotifications() {
      -        for (QuorumServer server : self.getVotingView().values()) {
      -            long sid = server.id;
      -
      +        for (long sid : self.getAllKnownServerIds()) {
      +            QuorumVerifier qv = self.getQuorumVerifier();
      

      Is that really desired? I suspect that is what's causing Observers to try to connect to each other (as opposed as just connecting to participants). I'll give it a try now and let you know. (Also, we use observer ids that are > 0, and I saw some parts of the code that might not deal with that assumption - so it could be that too..).

      1. notifications-loop.png
        44 kB
        Raul Gutierrez Segales
      2. ZOOKEEPER-1807.patch
        12 kB
        Raul Gutierrez Segales
      3. ZOOKEEPER-1807-alex.patch
        2 kB
        Alexander Shraer
      4. ZOOKEEPER-1807-ver2.patch
        10 kB
        Alexander Shraer
      5. ZOOKEEPER-1807-ver3.patch
        9 kB
        Alexander Shraer
      6. ZOOKEEPER-1807-ver4.patch
        10 kB
        Alexander Shraer
      7. ZOOKEEPER-1807-ver5.patch
        10 kB
        Alexander Shraer
      8. ZOOKEEPER-1807-ver6.patch
        24 kB
        Alexander Shraer
      9. ZOOKEEPER-1807-ver7.patch
        38 kB
        Alexander Shraer

        Issue Links

          Activity

          Hide
          Alexander Shraer added a comment -

          Hi Raul,

          ZK-107 allows changing server roles. In one config a server is an observer, in the next one it may be a follower. I haven't looked closely, but I think the intention was to talk with everyone you know to try to get the most up-to-date config information. Instead of reverting this to the previous code, consider adding a check (regardless of whether this is an observer/participant server) that won't attempt to create a connection if one is already there to the same server with the same election address (election addresses may change from one view to the next).

          The code should handle observer id > 0, please file a JIRA if you find that there is a problem somewhere.

          Thanks,
          Alex

          Show
          Alexander Shraer added a comment - Hi Raul, ZK-107 allows changing server roles. In one config a server is an observer, in the next one it may be a follower. I haven't looked closely, but I think the intention was to talk with everyone you know to try to get the most up-to-date config information. Instead of reverting this to the previous code, consider adding a check (regardless of whether this is an observer/participant server) that won't attempt to create a connection if one is already there to the same server with the same election address (election addresses may change from one view to the next). The code should handle observer id > 0, please file a JIRA if you find that there is a problem somewhere. Thanks, Alex
          Hide
          Raul Gutierrez Segales added a comment -

          Oh - fair enough. So I suspect QuorumCnxManager isn't doing the right thing then. Will take look. Thanks for the quick reply!

          Show
          Raul Gutierrez Segales added a comment - Oh - fair enough. So I suspect QuorumCnxManager isn't doing the right thing then. Will take look. Thanks for the quick reply!
          Hide
          Raul Gutierrez Segales added a comment -

          Actually - my initial assessment was wrong (the spammy "there is already a connection.." message confused me).I am seeing an excess in traffic between Observers through the election port, but it's not due to connection attempts. I'll come back with the actual messages. Sorry if this isn't actually related to ZOOKEEPER-107, Alexander Shraer.

          Show
          Raul Gutierrez Segales added a comment - Actually - my initial assessment was wrong (the spammy "there is already a connection.." message confused me).I am seeing an excess in traffic between Observers through the election port, but it's not due to connection attempts. I'll come back with the actual messages. Sorry if this isn't actually related to ZOOKEEPER-107 , Alexander Shraer .
          Hide
          Flavio Junqueira added a comment -

          It would be good to understand if this is a bug that affects the 3.4 branch as well and if it is a blocker, Raul Gutierrez Segales.

          Show
          Flavio Junqueira added a comment - It would be good to understand if this is a bug that affects the 3.4 branch as well and if it is a blocker, Raul Gutierrez Segales .
          Hide
          Raul Gutierrez Segales added a comment -

          Yes - absolutely Flavio Junqueira. The amount of traffic that I am seeing between Observers through the election port is... scary. I am still trying to figure out what is going on. Will be back in a bit when I have a proper analysis.

          Show
          Raul Gutierrez Segales added a comment - Yes - absolutely Flavio Junqueira . The amount of traffic that I am seeing between Observers through the election port is... scary. I am still trying to figure out what is going on. Will be back in a bit when I have a proper analysis.
          Hide
          Thawan Kooburat added a comment -

          In our internal deployment, the host list in zoo.cfg for each observer only have the participants and itself. This helps address this issue a bit but obviously, in 3.5 world, this won't work if you want to promote an observer to a participant.

          Show
          Thawan Kooburat added a comment - In our internal deployment, the host list in zoo.cfg for each observer only have the participants and itself. This helps address this issue a bit but obviously, in 3.5 world, this won't work if you want to promote an observer to a participant.
          Hide
          Raul Gutierrez Segales added a comment -

          Okey - this seems to actually be related to ZOOKEEPER-107, Alexander Shraer. I added some debugging logging and I've see that the spam, to all Observers, are the notifications:

          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 9, peerEpoch = 130, configData = [B@5a0c0ce6
          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 12, peerEpoch = 130, configData = [B@4d22fe39
          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 6, peerEpoch = 130, configData = [B@346077bf
          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 13, peerEpoch = 130, configData = [B@2955b776
          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 11, peerEpoch = 130, configData = [B@3a7fb92d
          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 14, peerEpoch = 130, configData = [B@1756575c
          2013-11-02 02:33:21,341 - INFO  [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 13, peerEpoch = 130, configData = [B@258164fc
          

          As you can see, it's sending tons of notifications per second. Not good

          With this diff in FastLeaderElection.java (i.e.: a revert of part of your change):

               private void sendNotifications() {
          -        for (long sid : self.getAllKnownServerIds()) {
          +        for (QuorumServer server : self.getVotingView().values()) {
          +            long sid = server.id;
          

          observers, of course, don't get spammed. I am guessing some condition is failing for Observers that assumes the notifications are fresh and sends them repeatedly?

          Show
          Raul Gutierrez Segales added a comment - Okey - this seems to actually be related to ZOOKEEPER-107 , Alexander Shraer . I added some debugging logging and I've see that the spam, to all Observers, are the notifications: 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 9, peerEpoch = 130, configData = [B@5a0c0ce6 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 12, peerEpoch = 130, configData = [B@4d22fe39 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 6, peerEpoch = 130, configData = [B@346077bf 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 13, peerEpoch = 130, configData = [B@2955b776 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 11, peerEpoch = 130, configData = [B@3a7fb92d 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 14, peerEpoch = 130, configData = [B@1756575c 2013-11-02 02:33:21,341 - INFO [WorkerSender[myid=13]] - will send: leader = 3, zxid = 558362464215, electionEpoch = 5, state = OBSERVING, sid = 13, peerEpoch = 130, configData = [B@258164fc As you can see, it's sending tons of notifications per second. Not good With this diff in FastLeaderElection.java (i.e.: a revert of part of your change): private void sendNotifications() { - for (long sid : self.getAllKnownServerIds()) { + for (QuorumServer server : self.getVotingView().values()) { + long sid = server.id; observers, of course, don't get spammed. I am guessing some condition is failing for Observers that assumes the notifications are fresh and sends them repeatedly?
          Hide
          Raul Gutierrez Segales added a comment -

          Flavio Junqueira: I think this is 3.5.0 specific since it goes away whilst reverting those bits from ZOOKEEPER-107 (there is a chance I am overlooking something, of course, and it's some other thing). But this is most likely a blocker for the 3.5.0 release though.

          Show
          Raul Gutierrez Segales added a comment - Flavio Junqueira : I think this is 3.5.0 specific since it goes away whilst reverting those bits from ZOOKEEPER-107 (there is a chance I am overlooking something, of course, and it's some other thing). But this is most likely a blocker for the 3.5.0 release though.
          Hide
          Raul Gutierrez Segales added a comment -

          Thawan Kooburat: should omitting the Observers from zoo.cfg actually make any difference? If so we should document it somewhere (unless it already is is). In my case, where I do explicitly enumerate them I don't get observers-to-observers connections on the election port once I remove the bits I mentioned above in FLE (so it seems to me it isn't).

          Show
          Raul Gutierrez Segales added a comment - Thawan Kooburat : should omitting the Observers from zoo.cfg actually make any difference? If so we should document it somewhere (unless it already is is). In my case, where I do explicitly enumerate them I don't get observers-to-observers connections on the election port once I remove the bits I mentioned above in FLE (so it seems to me it isn't).
          Hide
          Raul Gutierrez Segales added a comment -

          I think what's happening is that when we send the initial notifications to all members, as opposed to just voting members as it was before, we trigger off a self-replicating cascade of notifications. Each Observers gets the notification and then by virtue of:

                                  /*                                                                                                                            
                                   * If it is from a non-voting server (such as an observer or                                                                  
                                   * a non-voting follower), respond right away.                                                                                
                                   */
                                  if(!self.getVotingView().containsKey(response.sid)){
                                     .....
                                  }
          

          it replies back to each Observer and so on. So sounds to me that this needs to match what we have in sendNotifications and actually check response.sid against self.getAllKnownServerIds() to avoid the endless echoing of notifications that I am seeing.

          Thoughts Alexander Shraer, Flavio Junqueira ?

          Show
          Raul Gutierrez Segales added a comment - I think what's happening is that when we send the initial notifications to all members, as opposed to just voting members as it was before, we trigger off a self-replicating cascade of notifications. Each Observers gets the notification and then by virtue of: /* * If it is from a non-voting server (such as an observer or * a non-voting follower), respond right away. */ if(!self.getVotingView().containsKey(response.sid)){ ..... } it replies back to each Observer and so on. So sounds to me that this needs to match what we have in sendNotifications and actually check response.sid against self.getAllKnownServerIds() to avoid the endless echoing of notifications that I am seeing. Thoughts Alexander Shraer , Flavio Junqueira ?
          Hide
          Raul Gutierrez Segales added a comment -

          The attached patch prevents sending replies back when we are an Observer. Since ZOOKEEPER-107 we send notifications to Observers because they can be promoted to Participants. But to avoid replicating replies forver (i.e.: an observer sends a notification and the receiving observer then sends another one and so on) we don't have to send notifications when we are a LearnerType.OBSERVER.

          Show
          Raul Gutierrez Segales added a comment - The attached patch prevents sending replies back when we are an Observer. Since ZOOKEEPER-107 we send notifications to Observers because they can be promoted to Participants. But to avoid replicating replies forver (i.e.: an observer sends a notification and the receiving observer then sends another one and so on) we don't have to send notifications when we are a LearnerType.OBSERVER.
          Hide
          Hadoop QA added a comment -

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

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

          Thanks Raul. This seems like a bit of an overkill - you're eliminating observer to observer responses. Would be better to understand what causes it to spin and to send notifications in normal rate like for participants.

          Show
          Alexander Shraer added a comment - Thanks Raul. This seems like a bit of an overkill - you're eliminating observer to observer responses. Would be better to understand what causes it to spin and to send notifications in normal rate like for participants.
          Hide
          Raul Gutierrez Segales added a comment -

          Well, if we really need observer to observer responses, for reconfig purposes I presume, then should we be sending them to observers not in LOOKING state? See the conditions that apply when responding to participants in the lines below my patch.

          But even still with that being correct it might be too much overhead for large Observers deployments. Should this be optional?

          Show
          Raul Gutierrez Segales added a comment - Well, if we really need observer to observer responses, for reconfig purposes I presume, then should we be sending them to observers not in LOOKING state? See the conditions that apply when responding to participants in the lines below my patch. But even still with that being correct it might be too much overhead for large Observers deployments. Should this be optional?
          Hide
          Alexander Shraer added a comment -

          what if we remove the "if(!self.getVotingView().containsKey(response.sid)){"
          and always run the else code ?

          Show
          Alexander Shraer added a comment - what if we remove the "if(!self.getVotingView().containsKey(response.sid)){" and always run the else code ?
          Hide
          Alexander Shraer added a comment -

          regarding overhead - if I understand the "else" code correctly, it will only send a message if one of them is LOOKING, so I'm not sure that the overhead is excessive.

          Show
          Alexander Shraer added a comment - regarding overhead - if I understand the "else" code correctly, it will only send a message if one of them is LOOKING, so I'm not sure that the overhead is excessive.
          Hide
          Raul Gutierrez Segales added a comment -

          Yeah - I think you are right. In this ZOOKEEPER-107 world in which observers can be promoted, etc the initial if() doesn't make sense anymore. I'll submit a new patch so we can think about it a bit more.

          With regards of the overhead and making all of this optional, well if you have > 100 observers restarted at once you'll have a large of notifications traffic. But I guess within the limits of tolerable.

          Show
          Raul Gutierrez Segales added a comment - Yeah - I think you are right. In this ZOOKEEPER-107 world in which observers can be promoted, etc the initial if() doesn't make sense anymore. I'll submit a new patch so we can think about it a bit more. With regards of the overhead and making all of this optional, well if you have > 100 observers restarted at once you'll have a large of notifications traffic. But I guess within the limits of tolerable.
          Hide
          Raul Gutierrez Segales added a comment -

          As discussed with Alexander Shraer, we now need to apply the same response logic for voting and non-voting members.

          Flavio Junqueira, Thawan Kooburat - thoughts?

          Show
          Raul Gutierrez Segales added a comment - As discussed with Alexander Shraer , we now need to apply the same response logic for voting and non-voting members. Flavio Junqueira , Thawan Kooburat - thoughts?
          Hide
          Hadoop QA added a comment -

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

          +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 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/1735//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1735//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1735//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/12611781/ZOOKEEPER-1807.patch against trunk revision 1535491. +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 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/1735//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1735//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1735//console This message is automatically generated.
          Hide
          Raul Gutierrez Segales added a comment -

          Added a few minor cosmetic cleanups.

          My main concern is that even with this you would still have loops when a pair of observers is in LOOKING state. One way of cutting the loop would be fencing:

          recvqueue.offer(n);
          

          with:

          if (self.getLearnerType() == LearnerType.OBSERVER) { ... }
          

          but I am unsure how well that plays with all the valid config transitions that can happen with ZOOKEEPER-107.

          Show
          Raul Gutierrez Segales added a comment - Added a few minor cosmetic cleanups. My main concern is that even with this you would still have loops when a pair of observers is in LOOKING state. One way of cutting the loop would be fencing: recvqueue.offer(n); with: if (self.getLearnerType() == LearnerType.OBSERVER) { ... } but I am unsure how well that plays with all the valid config transitions that can happen with ZOOKEEPER-107 .
          Hide
          Alexander Shraer added a comment -

          probably there's not going to be any more of a loop than for participants.
          if you think this is not acceptable for observers, it would be sufficient to reply only when the sending server has a bigger config version (the one in QuorumVerifier) than the potential receiver. Otherwise there's no benefit for the receiver in terms of learning about new configs.

          Show
          Alexander Shraer added a comment - probably there's not going to be any more of a loop than for participants. if you think this is not acceptable for observers, it would be sufficient to reply only when the sending server has a bigger config version (the one in QuorumVerifier) than the potential receiver. Otherwise there's no benefit for the receiver in terms of learning about new configs.
          Hide
          Raul Gutierrez Segales added a comment -

          Thanks for the quick comment Alex. Yeah sounds to me that might be acceptable. Again, for huge deployments it might be a bit of concern since you'll be putting extra pressure on the cluster after, say, a big network partition. Thoughts? Cc: Thawan Kooburat, Flavio Junqueira.

          Show
          Raul Gutierrez Segales added a comment - Thanks for the quick comment Alex. Yeah sounds to me that might be acceptable. Again, for huge deployments it might be a bit of concern since you'll be putting extra pressure on the cluster after, say, a big network partition. Thoughts? Cc: Thawan Kooburat , Flavio Junqueira .
          Hide
          Hadoop QA added a comment -

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

          +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 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/1740//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1740//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1740//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/12611988/ZOOKEEPER-1807.patch against trunk revision 1535491. +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 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/1740//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1740//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1740//console This message is automatically generated.
          Hide
          Raul Gutierrez Segales added a comment -

          Here's how notification traffic (on election port 3888 in my case) goes down with the patch (i.e.: without the notifications loop). It's pretty dramatic so I'd say this is definitely a blocker for 3.5.0.

          Show
          Raul Gutierrez Segales added a comment - Here's how notification traffic (on election port 3888 in my case) goes down with the patch (i.e.: without the notifications loop). It's pretty dramatic so I'd say this is definitely a blocker for 3.5.0.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12611999/notifications-loop.png
          against trunk revision 1535491.

          +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/1741//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/12611999/notifications-loop.png against trunk revision 1535491. +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/1741//console This message is automatically generated.
          Hide
          Thawan Kooburat added a comment -

          I believe we have a much different concern using large number of observers. In our internal deployment, we did a few hacks which essentially kill all observer-to-observer communication. Observers only observe the result of election algorithm. We also add random delay when observer try to reconnect, so that participants has a chance to synchronize with the leader and form the quorum before the observers take away the leader's bandwidth.

          My understanding is that with our leader election algorithm, you need to broadcast your vote whenever your current vote change, so this will generate a lot of message during the initial phase of the algorithm. Also, N x N communication needed by LE is not going to scale for large deployment. For me, I don't think promoting observer to participant is going to be a common case (only needed for DR purpose), it would be acceptable to have optional flag to disable that feature in order to reduce LE overhead with large number of observers.

          Show
          Thawan Kooburat added a comment - I believe we have a much different concern using large number of observers. In our internal deployment, we did a few hacks which essentially kill all observer-to-observer communication. Observers only observe the result of election algorithm. We also add random delay when observer try to reconnect, so that participants has a chance to synchronize with the leader and form the quorum before the observers take away the leader's bandwidth. My understanding is that with our leader election algorithm, you need to broadcast your vote whenever your current vote change, so this will generate a lot of message during the initial phase of the algorithm. Also, N x N communication needed by LE is not going to scale for large deployment. For me, I don't think promoting observer to participant is going to be a common case (only needed for DR purpose), it would be acceptable to have optional flag to disable that feature in order to reduce LE overhead with large number of observers.
          Hide
          Alexander Shraer added a comment -

          Sorry for the confusion, everyone, but it seems that for reconfiguration purposes its only important to send a notification (containing new config) to a server if its a participant either in the current or in the next configuration. Only in that case we may need to convince him to adopt its new role as a participant and help form a quorum. So perhaps the attached patch could work. What do you think ?

          Show
          Alexander Shraer added a comment - Sorry for the confusion, everyone, but it seems that for reconfiguration purposes its only important to send a notification (containing new config) to a server if its a participant either in the current or in the next configuration. Only in that case we may need to convince him to adopt its new role as a participant and help form a quorum. So perhaps the attached patch could work. What do you think ?
          Hide
          Raul Gutierrez Segales added a comment -

          Btw - where's the canonical definition of how reconfiguration should work? Is it this implementation or are their docs somewhere else? If so, lets make sure that with this change we sync them up. If the behavior of reconfiguration is defined by the implementation then can we have more comments with this patch? I.e.: to who do we send notifications and what are we expecting, etc. Just to make sure that who ever hacks next on FLE doesn't lose sight of these constraints.

          Show
          Raul Gutierrez Segales added a comment - Btw - where's the canonical definition of how reconfiguration should work? Is it this implementation or are their docs somewhere else? If so, lets make sure that with this change we sync them up. If the behavior of reconfiguration is defined by the implementation then can we have more comments with this patch? I.e.: to who do we send notifications and what are we expecting, etc. Just to make sure that who ever hacks next on FLE doesn't lose sight of these constraints.
          Hide
          Alexander Shraer added a comment -

          This part is described in Section 3.2 of the paper: https://www.usenix.org/system/files/conference/atc12/atc12-final74.pdf
          Of course the paper doesn't talk about FastLeaderElection and things like that. So the actual implementation needs to have comments, and it does have them in many places, here we should probably explain some more.

          Show
          Alexander Shraer added a comment - This part is described in Section 3.2 of the paper: https://www.usenix.org/system/files/conference/atc12/atc12-final74.pdf Of course the paper doesn't talk about FastLeaderElection and things like that. So the actual implementation needs to have comments, and it does have them in many places, here we should probably explain some more.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12612023/ZOOKEEPER-1807-alex.patch
          against trunk revision 1535491.

          +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 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/1742//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1742//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1742//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/12612023/ZOOKEEPER-1807-alex.patch against trunk revision 1535491. +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 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/1742//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1742//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1742//console This message is automatically generated.
          Hide
          Germán Blanco added a comment -

          My son was playing with the keyboard yesterday and he assigned this JIRA to me. I hope this is the worst part of the mess.

          Show
          Germán Blanco added a comment - My son was playing with the keyboard yesterday and he assigned this JIRA to me. I hope this is the worst part of the mess.
          Hide
          Alexander Shraer added a comment -

          I was thinking about this some more but couldn't come up with a scenario that would require more than just sending notifications to the participants of the current view during FLE, like it was before ZK-107, so I'm reverting this change for now in the attached patch. During the process I added two tests that involve observers and reconfigurations, and found a small NullPointer bug in QuorumPeer which is also fixed here.

          Note that a leader would still have to contact both old and new view to commit a reconfig when it comes up, so its important that new observer/participants know who the chosen leader is, and I think they will know because we start them with a configuration containing the previous one and themselves, so they will initiate a connection to previous config servers and get FLE responses with leader info.

          The change made in ZK-107 quoted in this Jira had an issue that we're still only waiting for a quorum of the current view before terminating FLE, so all the extra messages may have just as well be lost or never sent... So either we wait for both old and new quorums in FLE or send just to old servers like before.

          Also, notice that the configuration being sent is the committed one, not the proposed one. So if a server A is a participant in the new view (maybe it was an observer in the old view but it doesn't matter), then anyone able to convince it that its a participant has already adopted the new config (knows that it was committed) and so sees A as a participant and will send a message to A even if it just sends the notification to all participants of its current view.

          Not complete sure I have this right, so if you think that I'm wrong please let me know. Benjamin Reed, Flavio Junqueira

          Show
          Alexander Shraer added a comment - I was thinking about this some more but couldn't come up with a scenario that would require more than just sending notifications to the participants of the current view during FLE, like it was before ZK-107, so I'm reverting this change for now in the attached patch. During the process I added two tests that involve observers and reconfigurations, and found a small NullPointer bug in QuorumPeer which is also fixed here. Note that a leader would still have to contact both old and new view to commit a reconfig when it comes up, so its important that new observer/participants know who the chosen leader is, and I think they will know because we start them with a configuration containing the previous one and themselves, so they will initiate a connection to previous config servers and get FLE responses with leader info. The change made in ZK-107 quoted in this Jira had an issue that we're still only waiting for a quorum of the current view before terminating FLE, so all the extra messages may have just as well be lost or never sent... So either we wait for both old and new quorums in FLE or send just to old servers like before. Also, notice that the configuration being sent is the committed one, not the proposed one. So if a server A is a participant in the new view (maybe it was an observer in the old view but it doesn't matter), then anyone able to convince it that its a participant has already adopted the new config (knows that it was committed) and so sees A as a participant and will send a message to A even if it just sends the notification to all participants of its current view. Not complete sure I have this right, so if you think that I'm wrong please let me know. Benjamin Reed , Flavio Junqueira
          Hide
          Hadoop QA added a comment -

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

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

          seems like I've already found and solved the same QuorumPeer NPE bug in ZOOKEEPER-1783, so once that one is committed I'll update the patch on this JIRA.

          Show
          Alexander Shraer added a comment - seems like I've already found and solved the same QuorumPeer NPE bug in ZOOKEEPER-1783 , so once that one is committed I'll update the patch on this JIRA.
          Hide
          Raul Gutierrez Segales added a comment -

          Alexander Shraer: do you think that, perhaps, adding a comment elaborating a bit more on the rationale of notifications and the state of the new/old config would be worthwhile? I am thinking the comment should be along sendNotifications().

          Show
          Raul Gutierrez Segales added a comment - Alexander Shraer : do you think that, perhaps, adding a comment elaborating a bit more on the rationale of notifications and the state of the new/old config would be worthwhile? I am thinking the comment should be along sendNotifications().
          Hide
          Raul Gutierrez Segales added a comment -

          (could we get a reviewboard for this? some inline comments below)

          For:

          +        // start server 3 with new config
          +        zk[2] = new ZooKeeper("127.0.0.1:" + ports[2][2], ClientBase.CONNECTION_TIMEOUT, this);
          

          I think the zk[2] assignment goes before the comment.

          For:

          +        for (int i=2; i<3; i++) {
          +            Assert.assertTrue("waiting for server "+ i + " being up",
          +                ClientBase.waitForServerUp("127.0.0.1:" + ports[i][2],
          +                        CONNECTION_TIMEOUT * 2));
          +            ReconfigTest.testServerHasConfig(zk[i], allServersNext, null);  
          +        }
          

          i<= 3? Or no loop if you only want it to loop one time I guess.

          Also the ports assignment loop and the currentQuorumCfgSection creation are repeated in testObserverConvertedToParticipantDuringFLE and testCurrentObserverIsParticipantInNewConfig; mind DRY-ing this up a bit by putting those in private methods? (i.e.: generatePorts() and generateInitialConfig() or such such).

          Show
          Raul Gutierrez Segales added a comment - (could we get a reviewboard for this? some inline comments below) For: + // start server 3 with new config + zk[2] = new ZooKeeper("127.0.0.1:" + ports[2][2], ClientBase.CONNECTION_TIMEOUT, this); I think the zk [2] assignment goes before the comment. For: + for (int i=2; i<3; i++) { + Assert.assertTrue("waiting for server "+ i + " being up", + ClientBase.waitForServerUp("127.0.0.1:" + ports[i][2], + CONNECTION_TIMEOUT * 2)); + ReconfigTest.testServerHasConfig(zk[i], allServersNext, null); + } i<= 3? Or no loop if you only want it to loop one time I guess. Also the ports assignment loop and the currentQuorumCfgSection creation are repeated in testObserverConvertedToParticipantDuringFLE and testCurrentObserverIsParticipantInNewConfig; mind DRY-ing this up a bit by putting those in private methods? (i.e.: generatePorts() and generateInitialConfig() or such such).
          Hide
          Hadoop QA added a comment -

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

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

          https://reviews.apache.org/r/15317/

          thanks for the comments, Raul.

          Flavio Junqueira Raul and others, please see if my comment makes sense

          Show
          Alexander Shraer added a comment - https://reviews.apache.org/r/15317/ thanks for the comments, Raul. Flavio Junqueira Raul and others, please see if my comment makes 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/12612664/ZOOKEEPER-1807-ver4.patch
          against trunk revision 1539529.

          +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/1750//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1750//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1750//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/12612664/ZOOKEEPER-1807-ver4.patch against trunk revision 1539529. +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/1750//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1750//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1750//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/12612698/ZOOKEEPER-1807-ver5.patch
          against trunk revision 1539529.

          +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/1751//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1751//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1751//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/12612698/ZOOKEEPER-1807-ver5.patch against trunk revision 1539529. +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/1751//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1751//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1751//console This message is automatically generated.
          Hide
          Raul Gutierrez Segales added a comment -

          I am happy to give the RB a shipit but I would prefer to have more feedback/reviews from Thawan Kooburat and Flavio Junqueira since they are more familiar with the internals of FLE.

          Show
          Raul Gutierrez Segales added a comment - I am happy to give the RB a shipit but I would prefer to have more feedback/reviews from Thawan Kooburat and Flavio Junqueira since they are more familiar with the internals of FLE.
          Hide
          Raul Gutierrez Segales added a comment -

          Guys, can we get some input here? cc: Thawan Kooburat, Flavio Junqueira. Happy to trade some review karma

          Show
          Raul Gutierrez Segales added a comment - Guys, can we get some input here? cc: Thawan Kooburat , Flavio Junqueira . Happy to trade some review karma
          Hide
          Michi Mutsuzaki added a comment -

          I changed the priority to 'blocker'. We should get this fixed in 3.5.0.

          Show
          Michi Mutsuzaki added a comment - I changed the priority to 'blocker'. We should get this fixed in 3.5.0.
          Hide
          Hadoop QA added a comment -

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

          +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/1971//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1971//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1971//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/12612698/ZOOKEEPER-1807-ver5.patch against trunk revision 1577756. +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/1971//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1971//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1971//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          +1 looks good to me. it would be nice if Flavio Junqueira gave it a glance though

          Show
          Benjamin Reed added a comment - +1 looks good to me. it would be nice if Flavio Junqueira gave it a glance though
          Hide
          Flavio Junqueira added a comment -

          There is probably something I'm missing here, but observers still need to receive notifications. They don't need to receive notifications from other observers, that's right, but we shouldn't be sending notifications only among the voting servers. If the server is voting in the view, then it should send to everyone. If the server is an observer, it should only receive from voting members. Does this logic sound right to you guys? Otherwise, let me know what I'm missing here.

          Show
          Flavio Junqueira added a comment - There is probably something I'm missing here, but observers still need to receive notifications. They don't need to receive notifications from other observers, that's right, but we shouldn't be sending notifications only among the voting servers. If the server is voting in the view, then it should send to everyone. If the server is an observer, it should only receive from voting members. Does this logic sound right to you guys? Otherwise, let me know what I'm missing here.
          Hide
          Raul Gutierrez Segales added a comment -

          I agree with your logic Flavio Junqueira but I don't see where the RB doesn't agree with you. Isn't Alex's change exactly what you say:

               private void sendNotifications() {
          -        for (long sid : self.getAllKnownServerIds()) {
          +        for (long sid : self.getVotingView().keySet()) {
          

          Only have voting members send notifications, but have everyone receive them. No?

          Show
          Raul Gutierrez Segales added a comment - I agree with your logic Flavio Junqueira but I don't see where the RB doesn't agree with you. Isn't Alex's change exactly what you say: private void sendNotifications() { - for (long sid : self.getAllKnownServerIds()) { + for (long sid : self.getVotingView().keySet()) { Only have voting members send notifications, but have everyone receive them. No?
          Hide
          Raul Gutierrez Segales added a comment -

          Maybe you looked at the initial comments, which implied that Observers shouldn't receive notifications? I think what we agreed of in the end is:

          https://reviews.apache.org/r/15317/

          Which I think aligns with what you said.

          Show
          Raul Gutierrez Segales added a comment - Maybe you looked at the initial comments, which implied that Observers shouldn't receive notifications? I think what we agreed of in the end is: https://reviews.apache.org/r/15317/ Which I think aligns with what you said.
          Hide
          Alexander Shraer added a comment -

          Hi Flavio, Raul,

          Befor ZK-107 the line was " for (QuorumServer server : self.getVotingView().values()) {"
          This patch basically brings this back. So if I understand correctly this wasn't sending notifications to observers before.
          But - everyone will send notifications to followers and if a follower receives a message it will respond directly, even to an observer. My reasoning is that FLE terminates once we have a quorum of the last committed config. So we could only
          possibly need votes from followers in the last committed config. Not from observers. Observers may contact followers through the same logic and get updated but this is not enforced by the termination rule of FLE. In the attached test the observer finds out that he really is a follower (whose vote is needed).

          Alex

          Show
          Alexander Shraer added a comment - Hi Flavio, Raul, Befor ZK-107 the line was " for (QuorumServer server : self.getVotingView().values()) {" This patch basically brings this back. So if I understand correctly this wasn't sending notifications to observers before. But - everyone will send notifications to followers and if a follower receives a message it will respond directly, even to an observer. My reasoning is that FLE terminates once we have a quorum of the last committed config. So we could only possibly need votes from followers in the last committed config. Not from observers. Observers may contact followers through the same logic and get updated but this is not enforced by the termination rule of FLE. In the attached test the observer finds out that he really is a follower (whose vote is needed). Alex
          Hide
          Flavio Junqueira added a comment -

          I'm actually getting it from the patch:

          self.getVotingView().keySet()
          
          Show
          Flavio Junqueira added a comment - I'm actually getting it from the patch: self.getVotingView().keySet()
          Hide
          Alexander Shraer added a comment -

          Flavio, I think Raul's comment actually doesn't reflect the latest patch, but if you read my last comment this may clarify things

          Show
          Alexander Shraer added a comment - Flavio, I think Raul's comment actually doesn't reflect the latest patch, but if you read my last comment this may clarify things
          Hide
          Flavio Junqueira added a comment -

          yeah, I probably don't have it fresh enough in my head. in FLE.lookForLeader(), we set the state of server looking like this:

          self.setPeerState((proposedLeader == self.getId()) ?
                                                  ServerState.LEADING: learningState());
          

          learning state can be following or observing. don't observers need to receive notifications too to be able to leave lookForLeader?

          Show
          Flavio Junqueira added a comment - yeah, I probably don't have it fresh enough in my head. in FLE.lookForLeader(), we set the state of server looking like this: self.setPeerState((proposedLeader == self.getId()) ? ServerState.LEADING: learningState()); learning state can be following or observing. don't observers need to receive notifications too to be able to leave lookForLeader?
          Hide
          Alexander Shraer added a comment -

          Yeah, and I think observers will indeed get a notification - they will sendNotifications to followers and the followers will respond directly to them. I meant that the termPredicate only needs followers to elect a leader.

          Show
          Alexander Shraer added a comment - Yeah, and I think observers will indeed get a notification - they will sendNotifications to followers and the followers will respond directly to them. I meant that the termPredicate only needs followers to elect a leader.
          Hide
          Raul Gutierrez Segales added a comment -

          Sorry guys - I totally mixed things up. FWIW, this is what we've been using for a couple of months now:

          diff --git a/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java b/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderEle
          index 9876c3d..38ae999 100644
          --- a/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
          +++ b/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
          @@ -582,7 +582,7 @@ public class FastLeaderElection implements Election {
                * Send notifications to all peers upon a change in our vote
                */
               private void sendNotifications() {
          -        for (long sid : self.getAllKnownServerIds()) {
          +        for (long sid : self.getCurrentAndNextConfigVoters()) {
                       QuorumVerifier qv = self.getQuorumVerifier();
                       ToSend notmsg = new ToSend(ToSend.mType.notification,
                               proposedLeader,
          diff --git a/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java b/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
          index 8926a82..06cf7d4 100644
          --- a/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
          +++ b/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
          @@ -1112,12 +1112,12 @@ public class QuorumPeer extends Thread implements QuorumStats.Provider {
                  return getQuorumVerifier().getObservingMembers();
               }
           
          -    public synchronized Set<Long> getAllKnownServerIds(){
          -       Set<Long> tmp = new HashSet<Long>(getQuorumVerifier().getAllMembers().keySet());
          +    public synchronized Set<Long> getCurrentAndNextConfigVoters(){
          +       Set<Long> voterIds = new HashSet<Long>(getQuorumVerifier().getVotingMembers().keySet());
                  if (getLastSeenQuorumVerifier()!=null) {
          -           tmp.addAll(getLastSeenQuorumVerifier().getAllMembers().keySet());
          +          voterIds.addAll(getLastSeenQuorumVerifier().getVotingMembers().keySet());
                  }
          -       return tmp;
          +       return voterIds;
               }
               
               /**
          
          Show
          Raul Gutierrez Segales added a comment - Sorry guys - I totally mixed things up. FWIW, this is what we've been using for a couple of months now: diff --git a/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java b/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderEle index 9876c3d..38ae999 100644 --- a/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java +++ b/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java @@ -582,7 +582,7 @@ public class FastLeaderElection implements Election { * Send notifications to all peers upon a change in our vote */ private void sendNotifications() { - for (long sid : self.getAllKnownServerIds()) { + for (long sid : self.getCurrentAndNextConfigVoters()) { QuorumVerifier qv = self.getQuorumVerifier(); ToSend notmsg = new ToSend(ToSend.mType.notification, proposedLeader, diff --git a/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java b/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java index 8926a82..06cf7d4 100644 --- a/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java +++ b/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java @@ -1112,12 +1112,12 @@ public class QuorumPeer extends Thread implements QuorumStats.Provider { return getQuorumVerifier().getObservingMembers(); } - public synchronized Set<Long> getAllKnownServerIds(){ - Set<Long> tmp = new HashSet<Long>(getQuorumVerifier().getAllMembers().keySet()); + public synchronized Set<Long> getCurrentAndNextConfigVoters(){ + Set<Long> voterIds = new HashSet<Long>(getQuorumVerifier().getVotingMembers().keySet()); if (getLastSeenQuorumVerifier()!=null) { - tmp.addAll(getLastSeenQuorumVerifier().getAllMembers().keySet()); + voterIds.addAll(getLastSeenQuorumVerifier().getVotingMembers().keySet()); } - return tmp; + return voterIds; } /**
          Hide
          Alexander Shraer added a comment -

          I think sending to just current config voters is good enough. I don't think Old->New notifications are needed:

          • if next config is not committed, no point in notifying them - we need the current config to come up anyway.
          • if next config is committed, they'll notify whoever's a voter in next config.

          Servers in current config only need to make sure they let all potential voters in current config know that they need their vote.

          I think the point is that observers will also execute sendNotifications, so they will talk with followers this way, just not with other observers.

          Show
          Alexander Shraer added a comment - I think sending to just current config voters is good enough. I don't think Old->New notifications are needed: if next config is not committed, no point in notifying them - we need the current config to come up anyway. if next config is committed, they'll notify whoever's a voter in next config. Servers in current config only need to make sure they let all potential voters in current config know that they need their vote. I think the point is that observers will also execute sendNotifications, so they will talk with followers this way, just not with other observers.
          Hide
          Flavio Junqueira added a comment -

          Participants passively send notifications to observers. In FLE, we have this when as server receives a notification:

                                  /*
                                   * If it is from a non-voting server (such as an observer or 
                                   * a non-voting follower), respond right away. 
                                   */
                                  if(!self.getVotingView().containsKey(response.sid)){
                                      Vote current = self.getCurrentVote();
                                      QuorumVerifier qv = self.getQuorumVerifier();
                                      ToSend notmsg = new ToSend(ToSend.mType.notification,
                                              current.getId(),
                                              current.getZxid(),
                                              logicalclock,
                                              self.getPeerState(),
                                              response.sid,
                                              current.getPeerEpoch(),
                                              qv.toString().getBytes());
          
                                      sendqueue.offer(notmsg);
          

          So it sounds fine if we do as Alex suggests: send only to participants of the current config.

          Show
          Flavio Junqueira added a comment - Participants passively send notifications to observers. In FLE, we have this when as server receives a notification: /* * If it is from a non-voting server (such as an observer or * a non-voting follower), respond right away. */ if(!self.getVotingView().containsKey(response.sid)){ Vote current = self.getCurrentVote(); QuorumVerifier qv = self.getQuorumVerifier(); ToSend notmsg = new ToSend(ToSend.mType.notification, current.getId(), current.getZxid(), logicalclock, self.getPeerState(), response.sid, current.getPeerEpoch(), qv.toString().getBytes()); sendqueue.offer(notmsg); So it sounds fine if we do as Alex suggests: send only to participants of the current config.
          Hide
          Alexander Shraer added a comment -

          Flavio Junqueira is that a +1 ?

          Show
          Alexander Shraer added a comment - Flavio Junqueira is that a +1 ?
          Hide
          Alexander Shraer added a comment -

          resubmitting to trigger automatic tests

          Show
          Alexander Shraer added a comment - resubmitting to trigger automatic tests
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12640553/ZOOKEEPER-1807-ver5.patch
          against trunk revision 1587818.

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

          The failing test raises a couple of interesting issues...
          Mainly I think there is a "race" between the completion of FLE where we only require a quorum of old config and the establishment of new leader where we'd need both old and new quorums if we're recovering from a failed reconfig. It looks like we should ensure that we at least have a quorum of new config before ending FLE and moving to the next stage where we actually need this quorum.

          Here are two scenarios where this seems important.

          Suppose we have A, B in old config and A, B, C, D, E in new one.
          Suppose A, B rebooted during reconfig and will now have to recover (commit or join the new config).

          Case 1 (the failing test): C, D, E committed the reconfig. If A and B don't establish connection to C, D, E before completing FLE they won't find out about the new config being committed and will continuously try and fail to complete the reconfig (they'll fail because they won't get a quorum of new config). Its sort of ok since C, D, E are up and running, and possibly C D E will eventually contact A and B, but perhaps we should avoid this scenario anyway. By ensuring that A,B talk with a quorum of new config during FLE we guarantee that they switch to new config and not try to establish a leader in old one.

          Case 2: if C, D, E hasn't committed the new config and are actually trying to connect to A and B, but A and B could complete FLE before hearing from C, D, E they may again end up giving up and returning to FLE because they have no quorum of new config.

          So perhaps we should send the notifications to new config too and enforce having a quorum of new config before FLE is complete...

          Show
          Alexander Shraer added a comment - The failing test raises a couple of interesting issues... Mainly I think there is a "race" between the completion of FLE where we only require a quorum of old config and the establishment of new leader where we'd need both old and new quorums if we're recovering from a failed reconfig. It looks like we should ensure that we at least have a quorum of new config before ending FLE and moving to the next stage where we actually need this quorum. Here are two scenarios where this seems important. Suppose we have A, B in old config and A, B, C, D, E in new one. Suppose A, B rebooted during reconfig and will now have to recover (commit or join the new config). Case 1 (the failing test): C, D, E committed the reconfig. If A and B don't establish connection to C, D, E before completing FLE they won't find out about the new config being committed and will continuously try and fail to complete the reconfig (they'll fail because they won't get a quorum of new config). Its sort of ok since C, D, E are up and running, and possibly C D E will eventually contact A and B, but perhaps we should avoid this scenario anyway. By ensuring that A,B talk with a quorum of new config during FLE we guarantee that they switch to new config and not try to establish a leader in old one. Case 2: if C, D, E hasn't committed the new config and are actually trying to connect to A and B, but A and B could complete FLE before hearing from C, D, E they may again end up giving up and returning to FLE because they have no quorum of new config. So perhaps we should send the notifications to new config too and enforce having a quorum of new config before FLE is complete...
          Hide
          Flavio Junqueira added a comment -

          possibly C D E will eventually contact A and B

          QCM in C D E will keep trying to send notifications to A B, no? If so, they will learn of the new config as soon as one of C D E connect to them. It doesn't seem so bad and perhaps it is not worth delaying FLE by enforcing a quorum of both old and new configs. What do you think?

          Show
          Flavio Junqueira added a comment - possibly C D E will eventually contact A and B QCM in C D E will keep trying to send notifications to A B, no? If so, they will learn of the new config as soon as one of C D E connect to them. It doesn't seem so bad and perhaps it is not worth delaying FLE by enforcing a quorum of both old and new configs. What do you think?
          Hide
          Flavio Junqueira added a comment -

          Ping, I don't think we have converged here yet, have we?

          Show
          Flavio Junqueira added a comment - Ping, I don't think we have converged here yet, have we?
          Hide
          Alexander Shraer added a comment -

          Flavio Junqueira, I'm trying to test your theory that new servers will continue to ping old ones until they connect. This scenario (I described in my previous message) comes up in the testNextConfigAlreadyActive in ReconfigRecoveryTest, which fails with the latest patch.

          it seems that servers 2 3 4 try to contact 0 and 1 but only once or twice and then stop trying. Do you know why this could be happening ? or where the retry logic implemented ? the log below is everything I get with respect to connection attempts, even if I wait longer.

          3 Opening channel to server 0
          2 Opening channel to server 0
          2 Cannot open channel to 0 at election address localhost/127.0.0.1:11223
          3 Cannot open channel to 0 at election address localhost/127.0.0.1:11223
          3 Opening channel to server 1
          2 Opening channel to server 1
          3 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          3 Opening channel to server 2
          2 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          3 Connected to server 2
          2 Opening channel to server 3
          2 Connected to server 3
          4 Opening channel to server 0
          4 Cannot open channel to 0 at election address localhost/127.0.0.1:11223
          4 Opening channel to server 1
          4 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          4 Opening channel to server 2
          4 Connected to server 2
          2 Opening channel to server 4
          2 Connected to server 4
          3 Opening channel to server 4
          3 Connected to server 4
          2 Opening channel to server 0
          4 Opening channel to server 3
          4 Connected to server 3
          2 Cannot open channel to 0 at election address localhost/127.0.0.1:11223
          2 Opening channel to server 1
          2 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          4 Opening channel to server 3
          4 Connected to server 3
          2 Opening channel to server 0
          2 Cannot open channel to 0 at election address localhost/127.0.0.1:11223
          2 Opening channel to server 1
          2 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          3 Opening channel to server 0
          3 Cannot open channel to 0 at election address localhost/127.0.0.1:11223
          3 Opening channel to server 1
          3 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          0 Opening channel to server 1
          0 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          0 Opening channel to server 1
          0 Cannot open channel to 1 at election address localhost/127.0.0.1:11226
          1 Opening channel to server 0
          1 Connected to server 0

          Show
          Alexander Shraer added a comment - Flavio Junqueira , I'm trying to test your theory that new servers will continue to ping old ones until they connect. This scenario (I described in my previous message) comes up in the testNextConfigAlreadyActive in ReconfigRecoveryTest, which fails with the latest patch. it seems that servers 2 3 4 try to contact 0 and 1 but only once or twice and then stop trying. Do you know why this could be happening ? or where the retry logic implemented ? the log below is everything I get with respect to connection attempts, even if I wait longer. 3 Opening channel to server 0 2 Opening channel to server 0 2 Cannot open channel to 0 at election address localhost/127.0.0.1:11223 3 Cannot open channel to 0 at election address localhost/127.0.0.1:11223 3 Opening channel to server 1 2 Opening channel to server 1 3 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 3 Opening channel to server 2 2 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 3 Connected to server 2 2 Opening channel to server 3 2 Connected to server 3 4 Opening channel to server 0 4 Cannot open channel to 0 at election address localhost/127.0.0.1:11223 4 Opening channel to server 1 4 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 4 Opening channel to server 2 4 Connected to server 2 2 Opening channel to server 4 2 Connected to server 4 3 Opening channel to server 4 3 Connected to server 4 2 Opening channel to server 0 4 Opening channel to server 3 4 Connected to server 3 2 Cannot open channel to 0 at election address localhost/127.0.0.1:11223 2 Opening channel to server 1 2 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 4 Opening channel to server 3 4 Connected to server 3 2 Opening channel to server 0 2 Cannot open channel to 0 at election address localhost/127.0.0.1:11223 2 Opening channel to server 1 2 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 3 Opening channel to server 0 3 Cannot open channel to 0 at election address localhost/127.0.0.1:11223 3 Opening channel to server 1 3 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 0 Opening channel to server 1 0 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 0 Opening channel to server 1 0 Cannot open channel to 1 at election address localhost/127.0.0.1:11226 1 Opening channel to server 0 1 Connected to server 0
          Hide
          Alexander Shraer added a comment -

          Latest patch implements the idea of sending notifications to current and next config followers, and accordingly also waiting for responses.

          Show
          Alexander Shraer added a comment - Latest patch implements the idea of sending notifications to current and next config followers, and accordingly also waiting for responses.
          Hide
          Hadoop QA added a comment -

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

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

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

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

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

          -1 findbugs. The patch appears to introduce 87 new Findbugs (version 2.0.3) 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/2196//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2196//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2196//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/12656256/ZOOKEEPER-1807-ver6.patch against trunk revision 1611309. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 87 new Findbugs (version 2.0.3) 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/2196//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2196//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2196//console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          I checked QCM and it indeed only keeps trying to connect while it is executing lookForLeader, so your observation is right. The approach you propose seems right to me.

          I also checked the test failure and the test that failed is NioNettySuiteHammerTest. It timed out, so it is hard to say if it is related to this patch or not, although I've seen this same test failure with another patch recently. I'm not sure what's causing it, so we might want to consider committing this patch and trying to figure out the hammer test problem separately.

          What do you think?

          Show
          Flavio Junqueira added a comment - I checked QCM and it indeed only keeps trying to connect while it is executing lookForLeader, so your observation is right. The approach you propose seems right to me. I also checked the test failure and the test that failed is NioNettySuiteHammerTest. It timed out, so it is hard to say if it is related to this patch or not, although I've seen this same test failure with another patch recently. I'm not sure what's causing it, so we might want to consider committing this patch and trying to figure out the hammer test problem separately. What do you think?
          Hide
          Alexander Shraer added a comment -

          Thanks for checking this. I ran the failing test locally twice and it passes. I'm not sure what happened with the findBugs failure, it doesn't look related to the patch, maybe a different version of findbugs started running ?
          if you think its ok, we could commit it.

          Show
          Alexander Shraer added a comment - Thanks for checking this. I ran the failing test locally twice and it passes. I'm not sure what happened with the findBugs failure, it doesn't look related to the patch, maybe a different version of findbugs started running ? if you think its ok, we could commit it.
          Hide
          Flavio Junqueira added a comment -

          We should get this in. The findbugs problem isn't related to this patch, and it is appearing in every patch that has been submitted in the past few days. I had a look at the findbugs report in any case and couldn't see anything related to this patch.

          Show
          Flavio Junqueira added a comment - We should get this in. The findbugs problem isn't related to this patch, and it is appearing in every patch that has been submitted in the past few days. I had a look at the findbugs report in any case and couldn't see anything related to this patch.
          Hide
          Flavio Junqueira added a comment -

          hey alex, would you mind cleaning up the patch a bit? There are some formatting problems and empty spaces, like:

          -        HashSet<Long> set = new HashSet<Long>();
          +      SyncedLearnerTracker voteSet = new SyncedLearnerTracker();
          +      voteSet.addQuorumVerifier(self.getQuorumVerifier());
          +      if (self.getLastSeenQuorumVerifier() != null 
          +              && self.getLastSeenQuorumVerifier().getVersion() > self.getQuorumVerifier().getVersion()) {
          +          voteSet.addQuorumVerifier(self.getLastSeenQuorumVerifier());
          +      }
          

          In this case there are more spaces for the hashset line than the others. There are also spurious spaces like at the end of the closing curly brace above, and some empty lines with spaces across the patch.

          Show
          Flavio Junqueira added a comment - hey alex, would you mind cleaning up the patch a bit? There are some formatting problems and empty spaces, like: - HashSet<Long> set = new HashSet<Long>(); + SyncedLearnerTracker voteSet = new SyncedLearnerTracker(); + voteSet.addQuorumVerifier(self.getQuorumVerifier()); + if (self.getLastSeenQuorumVerifier() != null + && self.getLastSeenQuorumVerifier().getVersion() > self.getQuorumVerifier().getVersion()) { + voteSet.addQuorumVerifier(self.getLastSeenQuorumVerifier()); + } In this case there are more spaces for the hashset line than the others. There are also spurious spaces like at the end of the closing curly brace above, and some empty lines with spaces across the patch.
          Hide
          Alexander Shraer added a comment -

          I re-formatted (using Eclipse's formatting) the new code, and the entire ReconfigRecoveryTest. Besides that the ver7 patch is the same as ver6.

          Show
          Alexander Shraer added a comment - I re-formatted (using Eclipse's formatting) the new code, and the entire ReconfigRecoveryTest. Besides that the ver7 patch is the same as ver6.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12656544/ZOOKEEPER-1807-ver7.patch
          against trunk revision 1611732.

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

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

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

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

          -1 findbugs. The patch appears to introduce 87 new Findbugs (version 2.0.3) 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/2199//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2199//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2199//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/12656544/ZOOKEEPER-1807-ver7.patch against trunk revision 1611732. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 87 new Findbugs (version 2.0.3) 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/2199//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2199//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2199//console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          +1, thanks, Alexander Shraer. Committed revision 1611765.

          Show
          Flavio Junqueira added a comment - +1, thanks, Alexander Shraer . Committed revision 1611765.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in ZooKeeper-trunk #2378 (See https://builds.apache.org/job/ZooKeeper-trunk/2378/)
          ZOOKEEPER-1807. Observers spam each other creating connections to the election addr (Alex Shraer via fpj) (fpj: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1611765)

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/ReconfigRecoveryTest.java
          Show
          Hudson added a comment - SUCCESS: Integrated in ZooKeeper-trunk #2378 (See https://builds.apache.org/job/ZooKeeper-trunk/2378/ ) ZOOKEEPER-1807 . Observers spam each other creating connections to the election addr (Alex Shraer via fpj) (fpj: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1611765 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/ReconfigRecoveryTest.java
          Show
          Patrick Hunt added a comment - Flavio Junqueira and Alexander Shraer please take a look at the recent test failure, a test introduced by this jira: https://builds.apache.org/view/S-Z/view/ZooKeeper/job/ZooKeeper-trunk/2380/testReport/junit/org.apache.zookeeper.server.quorum/ReconfigRecoveryTest/testCurrentObserverIsParticipantInNewConfig/
          Hide
          Flavio Junqueira added a comment -

          Reopening this issue due to test case failure

          Show
          Flavio Junqueira added a comment - Reopening this issue due to test case failure
          Hide
          Rakesh R added a comment -

          Any update on this issue as this issue is marked as blocker for 3.5.1.

          Show
          Rakesh R added a comment - Any update on this issue as this issue is marked as blocker for 3.5.1.
          Hide
          Raul Gutierrez Segales added a comment -

          Was the build failure transient? The build logs are gone...

          Show
          Raul Gutierrez Segales added a comment - Was the build failure transient? The build logs are gone...
          Hide
          Michi Mutsuzaki added a comment -

          I haven't seen this failure for a while, but I'm not sure anything was done to fix this. How about moving this to 3.5.2?

          Show
          Michi Mutsuzaki added a comment - I haven't seen this failure for a while, but I'm not sure anything was done to fix this. How about moving this to 3.5.2?
          Hide
          Michi Mutsuzaki added a comment -

          i take that back, here is a failure from 2 days ago: https://builds.apache.org/view/S-Z/view/ZooKeeper/job/ZooKeeper-trunk/2654/

          Show
          Michi Mutsuzaki added a comment - i take that back, here is a failure from 2 days ago: https://builds.apache.org/view/S-Z/view/ZooKeeper/job/ZooKeeper-trunk/2654/
          Hide
          Alexander Shraer added a comment -

          The test consistently passes locally for me, and I don't have access to the build machine so I'm not sure how to debug this.
          Does it fail for anyone else ?

          One interesting thing (although probably unrelated) is that there is "Processing stat command" 3000 times in the log. Are so many stat invocations expected ?

          Show
          Alexander Shraer added a comment - The test consistently passes locally for me, and I don't have access to the build machine so I'm not sure how to debug this. Does it fail for anyone else ? One interesting thing (although probably unrelated) is that there is "Processing stat command" 3000 times in the log. Are so many stat invocations expected ?

            People

            • Assignee:
              Alexander Shraer
              Reporter:
              Raul Gutierrez Segales
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:

                Development