Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.5.0
    • Fix Version/s: 3.5.1, 3.6.0
    • Component/s: tests
    • Labels:
      None

      Description

      testPortChange changes all ports and role of the server and thus causes existing clients to disconnect, while this wouldn't happen if only the client port changes. Need to fix it to only change client port and not all the other parameters and make sure that the clients don't disconnect, while new clients shouldn't be able to connect to the old port.

      1. ZOOKEEPER-2000.patch
        6 kB
        Alexander Shraer
      2. ZOOKEEPER-2000.patch
        6 kB
        Michi Mutsuzaki

        Issue Links

          Activity

          Alexander Shraer created issue -
          Alexander Shraer made changes -
          Field Original Value New Value
          Summary Avoid disconnecting existing clients on client port change Fix
          Alexander Shraer made changes -
          Summary Fix Fix ReconfigTest.testPortChange to keep
          Alexander Shraer made changes -
          Summary Fix ReconfigTest.testPortChange to keep Fix ReconfigTest.testPortChange
          Alexander Shraer made changes -
          Description Currently when a reconfiguration changes the client port / address of a server, clients connected to that server will be disconnected. It may be possible to avoid this.

          The relevant code is in NIOServerCnxmFactory.java, reconfigure() and NettyServerCnxnFactory.java reconfigure()
          testPortChange changes all ports and role of the server and thus causes existing clients to disconnect, while this wouldn't happen if only the client port changes. Need to fix it to only change client port and not all the other parameters and make sure that the clients don't disconnect, while new clients shouldn't be able to connect to the old port.
          Alexander Shraer made changes -
          Priority Major [ 3 ] Minor [ 4 ]
          Alexander Shraer made changes -
          Component/s tests [ 12312427 ]
          Alexander Shraer made changes -
          Assignee Alexander Shraer [ shralex ]
          Alexander Shraer made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Alexander Shraer made changes -
          Fix Version/s 3.5.0 [ 12316644 ]
          Alexander Shraer made changes -
          Attachment ZOOKEEPER-2000.patch [ 12658866 ]
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 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 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/2250//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2250//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2250//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/12658866/ZOOKEEPER-2000.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 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/2250//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2250//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2250//console This message is automatically generated.
          Patrick Hunt made changes -
          Fix Version/s 3.5.1 [ 12326786 ]
          Fix Version/s 3.5.0 [ 12316644 ]
          Hide
          Hongchao Deng added a comment -
                              public void process(WatchedEvent event) {}});
                  for (int i = 0; i < 10; i++) {
                      try {
                          Thread.sleep(1000);
                          zkArr[followerIndex].setData("/test", "teststr".getBytes(), -1);
                          Assert.fail("New client connected to old client port!");
                      } catch (KeeperException.ConnectionLossException e) {
                      }
          
          

          code format.

          Show
          Hongchao Deng added a comment - public void process(WatchedEvent event) {}}); for ( int i = 0; i < 10; i++) { try { Thread .sleep(1000); zkArr[followerIndex].setData( "/test" , "teststr" .getBytes(), -1); Assert.fail( "New client connected to old client port!" ); } catch (KeeperException.ConnectionLossException e) { } code format.
          Hide
          Hongchao Deng added a comment -

          +1, Patch looks good.
          It would better to cover quorum port change.

          Show
          Hongchao Deng added a comment - +1, Patch looks good. It would better to cover quorum port change.
          Hide
          Alexander Shraer added a comment -

          thanks Hongchao. the same test changes all three ports, one at a time.

          Show
          Alexander Shraer added a comment - thanks Hongchao. the same test changes all three ports, one at a time.
          Hide
          Hongchao Deng added a comment -

          Right.
          I missed this line

                  quorumPort = PortAssignment.unique();
          

          Better to use "newQuorumPort"??

          Show
          Hongchao Deng added a comment - Right. I missed this line quorumPort = PortAssignment.unique(); Better to use "newQuorumPort"??
          Michi Mutsuzaki made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Michi Mutsuzaki made changes -
          Attachment ZOOKEEPER-2000.patch [ 12659564 ]
          Michi Mutsuzaki made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Michi Mutsuzaki added a comment -

          Addressed Hongchao's comment. I'll check this in once the build passes.

          By the way, is this for 3.5.0 or 3.5.1? I think this can go into 3.5.0.

          Show
          Michi Mutsuzaki added a comment - Addressed Hongchao's comment. I'll check this in once the build passes. By the way, is this for 3.5.0 or 3.5.1? I think this can go into 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/12659564/ZOOKEEPER-2000.patch
          against trunk revision 1615240.

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

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

          Thanks a lot, Michi! I think this should be for 3.5.0.

          Show
          Alexander Shraer added a comment - Thanks a lot, Michi! I think this should be 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/12659564/ZOOKEEPER-2000.patch
          against trunk revision 1615240.

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

          +1 tests included. The patch appears to include 7 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 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/2263//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2263//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2263//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/12659564/ZOOKEEPER-2000.patch against trunk revision 1615240. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 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/2263//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2263//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2263//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/12659564/ZOOKEEPER-2000.patch
          against trunk revision 1615240.

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

          +1 tests included. The patch appears to include 7 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 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/2264//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2264//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2264//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/12659564/ZOOKEEPER-2000.patch against trunk revision 1615240. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 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/2264//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2264//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2264//console This message is automatically generated.
          Michi Mutsuzaki made changes -
          Fix Version/s 3.5.0 [ 12316644 ]
          Fix Version/s 3.5.1 [ 12326786 ]
          Show
          Michi Mutsuzaki added a comment - trunk: http://svn.apache.org/viewvc?view=revision&revision=1619030
          Michi Mutsuzaki made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          FAILURE: Integrated in ZooKeeper-trunk #2412 (See https://builds.apache.org/job/ZooKeeper-trunk/2412/)
          ZOOKEEPER-2000. Fix ReconfigTest.testPortChange (Alexander Shraer via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1619030)

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ReconfigTest.java
          Show
          Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2412 (See https://builds.apache.org/job/ZooKeeper-trunk/2412/ ) ZOOKEEPER-2000 . Fix ReconfigTest.testPortChange (Alexander Shraer via michim) (michim: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1619030 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ReconfigTest.java
          Show
          Michi Mutsuzaki added a comment - branch-3.5: http://svn.apache.org/viewvc?view=revision&revision=1619278
          Michi Mutsuzaki made changes -
          Description testPortChange changes all ports and role of the server and thus causes existing clients to disconnect, while this wouldn't happen if only the client port changes. Need to fix it to only change client port and not all the other parameters and make sure that the clients don't disconnect, while new clients shouldn't be able to connect to the old port.           testPortChange changes all ports and role of the server and thus causes existing clients to disconnect, while this wouldn't happen if only the client port changes. Need to fix it to only change client port and not all the other parameters and make sure that the clients don't disconnect, while new clients shouldn't be able to connect to the old port.
          Hide
          Patrick Hunt added a comment -

          Since this change was committed the upstream jenkins trunk test has been failing intermittently for this test. Please investigate.

          e.g.: https://builds.apache.org/view/S-Z/view/ZooKeeper/job/ZooKeeper-trunk/2420/testReport/junit/org.apache.zookeeper.test/ReconfigTest/testPortChange/

          Show
          Patrick Hunt added a comment - Since this change was committed the upstream jenkins trunk test has been failing intermittently for this test. Please investigate. e.g.: https://builds.apache.org/view/S-Z/view/ZooKeeper/job/ZooKeeper-trunk/2420/testReport/junit/org.apache.zookeeper.test/ReconfigTest/testPortChange/
          Patrick Hunt made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Hongchao Deng added a comment -

          I looked at a few other testPortChange failures and they have something in common – they all failed in changing the leader's port, which enforces another leader election.

          Alexander Shraer Does the completion of reconfig guarantee that a new leader having been elected?

          Show
          Hongchao Deng added a comment - I looked at a few other testPortChange failures and they have something in common – they all failed in changing the leader's port, which enforces another leader election. Alexander Shraer Does the completion of reconfig guarantee that a new leader having been elected?
          Hide
          Alexander Shraer added a comment -

          no, the operation completes when the commit&activate message is sent. This message may cause a leader election, but the reconfig operation may have completed by then.

          The error "expected:<test[1]> but was:<test[0]>" seems like a safety issue - data is being read but its the wrong data, this shouldn't happen.

          Show
          Alexander Shraer added a comment - no, the operation completes when the commit&activate message is sent. This message may cause a leader election, but the reconfig operation may have completed by then. The error "expected:<test [1] > but was:<test [0] >" seems like a safety issue - data is being read but its the wrong data, this shouldn't happen.
          Hide
          Alexander Shraer added a comment -

          btw changing leader port will always cause a leader re-election by design - it doesn't mean that the leader port change failed.

          Show
          Alexander Shraer added a comment - btw changing leader port will always cause a leader re-election by design - it doesn't mean that the leader port change failed.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Could there be a race between multiple clients because ports are changed?

          Show
          Hongchao Deng added a comment - Alexander Shraer Could there be a race between multiple clients because ports are changed?
          Patrick Hunt made changes -
          Fix Version/s 3.5.1 [ 12326786 ]
          Fix Version/s 3.6.0 [ 12326518 ]
          Fix Version/s 3.5.0 [ 12316644 ]
          Hide
          Michi Mutsuzaki added a comment -

          Hi Alex, is this patch good to go? I'm seeing this test fail once in a while in the precommit build:

          https://builds.apache.org/view/S-Z/view/ZooKeeper/job/PreCommit-ZOOKEEPER-Build/2389/

          Show
          Michi Mutsuzaki added a comment - Hi Alex, is this patch good to go? I'm seeing this test fail once in a while in the precommit build: https://builds.apache.org/view/S-Z/view/ZooKeeper/job/PreCommit-ZOOKEEPER-Build/2389/
          Lam Thien Son made changes -
          Link This issue is blocked by LOGGING-160 [ LOGGING-160 ]

            People

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

              Dates

              • Created:
                Updated:

                Development