ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1293

Remove unused readyToStart from Leader.java

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: server, tests
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      After ZOOKEEPER-1194 readyToStart is no longer used.

      1. ZOOKEEPER-1293-ver2.patch
        2 kB
        Alexander Shraer
      2. ZOOKEEPER-1293-ver2.patch
        2 kB
        Alexander Shraer
      3. ZOOKEEPER-1293.patch
        2 kB
        Alexander Shraer

        Activity

        Hide
        Patrick Hunt added a comment -

        No thanks necessary. Thank you for contributing. Regards.

        Show
        Patrick Hunt added a comment - No thanks necessary. Thank you for contributing. Regards.
        Hide
        Alexander Shraer added a comment -

        thanks Patrick!

        Show
        Alexander Shraer added a comment - thanks Patrick!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1419 (See https://builds.apache.org/job/ZooKeeper-trunk/1419/)
        ZOOKEEPER-1293. Remove unused readyToStart from Leader.java (Alex Shraer via phunt)

        phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1227927
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1419 (See https://builds.apache.org/job/ZooKeeper-trunk/1419/ ) ZOOKEEPER-1293 . Remove unused readyToStart from Leader.java (Alex Shraer via phunt) phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1227927 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/Leader.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java
        Hide
        Patrick Hunt added a comment -

        lgtm, thanks Alexander!

        Show
        Patrick Hunt added a comment - lgtm, thanks Alexander!
        Hide
        Hadoop QA added a comment -

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

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

        Perhaps this would be a good place/time to refactor the code a bit, exposing a method from Leader rather than accessing fields of Leader directly? You could verify the synchronization as part of the fix. (unfortunately I didn't have time to look at it in more depth myself).

        Show
        Patrick Hunt added a comment - Perhaps this would be a good place/time to refactor the code a bit, exposing a method from Leader rather than accessing fields of Leader directly? You could verify the synchronization as part of the fix. (unfortunately I didn't have time to look at it in more depth myself).
        Hide
        Alexander Shraer added a comment -

        wrt to ZK-1194 I don't believe there was a bug introduced since we use synchronized for connectingFollowers and electingFollowers

        Show
        Alexander Shraer added a comment - wrt to ZK-1194 I don't believe there was a bug introduced since we use synchronized for connectingFollowers and electingFollowers
        Hide
        Alexander Shraer added a comment -

        I see. I agree that this creates a race on cnxAcceptor (although in practice I don't think this is harmful in this case since its used just to check whether to proceed or invoke another Thread.sleep(20)).

        We could just make cnxAcceptor volatile ?

        Show
        Alexander Shraer added a comment - I see. I agree that this creates a race on cnxAcceptor (although in practice I don't think this is harmful in this case since its used just to check whether to proceed or invoke another Thread.sleep(20)). We could just make cnxAcceptor volatile ?
        Hide
        Patrick Hunt added a comment -

        Hi Alexander, yes, I get that. My point though was that readyToStart was volatile, while cnxAcceptor is not. Leader is setting cnxAcceptor just prior to setting readyToStart.

        My concern is that cnxAcceptor will be null from the view of any other thread than the leader, given there is no longer any explicit barrier. Is this not correct?

        Show
        Patrick Hunt added a comment - Hi Alexander, yes, I get that. My point though was that readyToStart was volatile, while cnxAcceptor is not. Leader is setting cnxAcceptor just prior to setting readyToStart. My concern is that cnxAcceptor will be null from the view of any other thread than the leader, given there is no longer any explicit barrier. Is this not correct?
        Hide
        Alexander Shraer added a comment -

        by "better" I mean that the test is now more conservative

        Show
        Alexander Shraer added a comment - by "better" I mean that the test is now more conservative
        Hide
        Alexander Shraer added a comment -

        Hi Patrick,

        readyToStart was used in waitForEpochAck to check that the leader passed a certain point in the code (right before getEpochToPropose), and this check was not sufficient because it doesn't mean that the leader reached the getEpochToPropose or the waitForEpochAck barrier. ZK-1194 fixed this by explicitly checking that the leader reached the right barrier, so it no longer checks readyToStart.

        The only place where readyToStart is still used is in Zab1_0Test.java, where it is read to check that Leader.lead() passed cnxAcceptor.start(). As far as I understand, checking that leader.cnxAcceptor.isAlive() achieves the same (it might actually be better if cnxAcceptor.start() doesn't atomically make cnxAcceptor alive).

        Happy New Year!
        Alex

        Show
        Alexander Shraer added a comment - Hi Patrick, readyToStart was used in waitForEpochAck to check that the leader passed a certain point in the code (right before getEpochToPropose), and this check was not sufficient because it doesn't mean that the leader reached the getEpochToPropose or the waitForEpochAck barrier. ZK-1194 fixed this by explicitly checking that the leader reached the right barrier, so it no longer checks readyToStart. The only place where readyToStart is still used is in Zab1_0Test.java, where it is read to check that Leader.lead() passed cnxAcceptor.start(). As far as I understand, checking that leader.cnxAcceptor.isAlive() achieves the same (it might actually be better if cnxAcceptor.start() doesn't atomically make cnxAcceptor alive). Happy New Year! Alex
        Hide
        Patrick Hunt added a comment -

        I didn't dig deep, but readyToStart was volatile, cnxAcceptor is not, isn't that a problem? How about in the context of ZOOKEEPER-1194? (I mean, was a bug introduced there wrt replacing the ref to readyToStart)

        Show
        Patrick Hunt added a comment - I didn't dig deep, but readyToStart was volatile, cnxAcceptor is not, isn't that a problem? How about in the context of ZOOKEEPER-1194 ? (I mean, was a bug introduced there wrt replacing the ref to readyToStart)
        Hide
        Hadoop QA added a comment -

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

        +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/871//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/871//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/871//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/12503020/ZOOKEEPER-1293.patch against trunk revision 1225352. +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/871//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/871//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/871//console This message is automatically generated.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development