Details

    • Type: Sub-task Sub-task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      ZooDefs.OpCode is an interface with integer constants. Changing this to an enum provides safety. See "Item 30: Use enums instead of int constants" in Effective Java.

      1. ZOOKEEPER-1199.patch
        108 kB
        Thomas Koch
      2. ZOOKEEPER-1199.patch
        108 kB
        Thomas Koch
      3. ZOOKEEPER-1199.patch
        107 kB
        Thomas Koch
      4. ZOOKEEPER-1199.patch
        106 kB
        Thomas Koch
      5. ZOOKEEPER-1199.patch
        100 kB
        Thomas Koch
      6. ZOOKEEPER-1199.patch
        100 kB
        Thomas Koch

        Activity

        Hide
        Thomas Koch added a comment -

        There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in?
        I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there?

        Show
        Thomas Koch added a comment - There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in? I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2015/
        -----------------------------------------------------------

        Review request for zookeeper.

        Summary
        -------

        There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in?
        I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there?

        This addresses bug ZOOKEEPER-1199.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1199

        Diffs


        src/java/main/org/apache/zookeeper/ClientCnxn.java db15348
        src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 017ab14
        src/java/main/org/apache/zookeeper/MultiResponse.java 97d4c7d
        src/java/main/org/apache/zookeeper/MultiTransactionRecord.java af3b58d
        src/java/main/org/apache/zookeeper/Op.java 3c3db2e
        src/java/main/org/apache/zookeeper/OpResult.java 514318f
        src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20
        src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0
        src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8
        src/java/main/org/apache/zookeeper/server/DataTree.java 3d0b3c4
        src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java c746299
        src/java/main/org/apache/zookeeper/server/LogFormatter.java 9be3fe3
        src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 9e55c7b
        src/java/main/org/apache/zookeeper/server/Request.java 53631f1
        src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b
        src/java/main/org/apache/zookeeper/server/TraceFormatter.java 60d1cc7
        src/java/main/org/apache/zookeeper/server/ZKDatabase.java 92f475b
        src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 9ccaa0e
        src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java fec70de
        src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java a0c2cbd
        src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java a1c8ce2
        src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java 95c77b5
        src/java/main/org/apache/zookeeper/server/quorum/Learner.java a97a543
        src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java c518792
        src/java/main/org/apache/zookeeper/server/quorum/LearnerSyncRequest.java bfbc9a8
        src/java/main/org/apache/zookeeper/server/quorum/Observer.java 2e15a81
        src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java e94414f
        src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java 82d1468
        src/java/main/org/apache/zookeeper/server/upgrade/DataTreeV1.java 8559451
        src/java/main/org/apache/zookeeper/server/upgrade/UpgradeSnapShotV1.java 1ca9f9a
        src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java f9647c4
        src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java e37cbec
        src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java be29939
        src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java a134210

        Diff: https://reviews.apache.org/r/2015/diff

        Testing
        -------

        Thanks,

        Thomas

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2015/ ----------------------------------------------------------- Review request for zookeeper. Summary ------- There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in? I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there? This addresses bug ZOOKEEPER-1199 . https://issues.apache.org/jira/browse/ZOOKEEPER-1199 Diffs src/java/main/org/apache/zookeeper/ClientCnxn.java db15348 src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 017ab14 src/java/main/org/apache/zookeeper/MultiResponse.java 97d4c7d src/java/main/org/apache/zookeeper/MultiTransactionRecord.java af3b58d src/java/main/org/apache/zookeeper/Op.java 3c3db2e src/java/main/org/apache/zookeeper/OpResult.java 514318f src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20 src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8 src/java/main/org/apache/zookeeper/server/DataTree.java 3d0b3c4 src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java c746299 src/java/main/org/apache/zookeeper/server/LogFormatter.java 9be3fe3 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 9e55c7b src/java/main/org/apache/zookeeper/server/Request.java 53631f1 src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b src/java/main/org/apache/zookeeper/server/TraceFormatter.java 60d1cc7 src/java/main/org/apache/zookeeper/server/ZKDatabase.java 92f475b src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 9ccaa0e src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java fec70de src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java a0c2cbd src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java a1c8ce2 src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java 95c77b5 src/java/main/org/apache/zookeeper/server/quorum/Learner.java a97a543 src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java c518792 src/java/main/org/apache/zookeeper/server/quorum/LearnerSyncRequest.java bfbc9a8 src/java/main/org/apache/zookeeper/server/quorum/Observer.java 2e15a81 src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java e94414f src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java 82d1468 src/java/main/org/apache/zookeeper/server/upgrade/DataTreeV1.java 8559451 src/java/main/org/apache/zookeeper/server/upgrade/UpgradeSnapShotV1.java 1ca9f9a src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java f9647c4 src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java e37cbec src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java be29939 src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java a134210 Diff: https://reviews.apache.org/r/2015/diff Testing ------- Thanks, Thomas
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/578//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/578//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/578//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/12496091/ZOOKEEPER-1199.patch against trunk revision 1173949. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/578//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/578//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/578//console This message is automatically generated.
        Hide
        Benjamin Reed added a comment -

        -1 i don't think this is a valid issue. the op code evaluation is in time critical code and we should be doing less type conversion rather than more. it would be much better to write a utility class that converts from op code integer to string.

        Show
        Benjamin Reed added a comment - -1 i don't think this is a valid issue. the op code evaluation is in time critical code and we should be doing less type conversion rather than more. it would be much better to write a utility class that converts from op code integer to string.
        Hide
        Thomas Koch added a comment -

        Do you have some reference to back your concerns about a possible performance impact of some Type conversions? I've been taught that nowadays you should not micro-optimize your code because CPUs are fast enough. JVMs are also very clever in doing optimizations for you.
        Then you normally need to worry about disc and network performance, CPUs are typically not the bottleneck. Isn't it right, that the time that a ZK requests spends on the network is some orders of magnitude longer then the total CPU time for one request?

        Are there some numbers from Yahoo, which I could study to get a picture from larger installations?

        Then there's programmers time vs. machine time. A bigger machine is usually cheaper then more developers. (This may become wrong when talking about Hadoop scale, but ZK is only deployed to a handful of machines.)

        It would be good, if we could have some automatic evaluation of the performance impact as part of the Jenkins build...?

        Show
        Thomas Koch added a comment - Do you have some reference to back your concerns about a possible performance impact of some Type conversions? I've been taught that nowadays you should not micro-optimize your code because CPUs are fast enough. JVMs are also very clever in doing optimizations for you. Then you normally need to worry about disc and network performance, CPUs are typically not the bottleneck. Isn't it right, that the time that a ZK requests spends on the network is some orders of magnitude longer then the total CPU time for one request? Are there some numbers from Yahoo, which I could study to get a picture from larger installations? Then there's programmers time vs. machine time. A bigger machine is usually cheaper then more developers. (This may become wrong when talking about Hadoop scale, but ZK is only deployed to a handful of machines.) It would be good, if we could have some automatic evaluation of the performance impact as part of the Jenkins build...?
        Hide
        Thomas Koch added a comment -

        retrigger the build. Maybe there was sth. with Jenkins because it was just restarted.

        Show
        Thomas Koch added a comment - retrigger the build. Maybe there was sth. with Jenkins because it was just restarted.
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/580//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/580//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/580//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/12496126/ZOOKEEPER-1199.patch against trunk revision 1173949. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/580//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/580//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/580//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/12500448/ZOOKEEPER-1199.patch
        against trunk revision 1188039.

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/643//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/643//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/643//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/12500448/ZOOKEEPER-1199.patch against trunk revision 1188039. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/643//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/643//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/643//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/12500458/ZOOKEEPER-1199.patch
        against trunk revision 1188039.

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

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

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

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/647//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/647//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/647//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/12500458/ZOOKEEPER-1199.patch against trunk revision 1188039. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/647//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/647//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/647//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/12500490/ZOOKEEPER-1199.patch
        against trunk revision 1188039.

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

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

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2015/
        -----------------------------------------------------------

        (Updated 2011-10-24 17:44:19.879262)

        Review request for zookeeper.

        Summary
        -------

        There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in?
        I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there?

        This addresses bug ZOOKEEPER-1199.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1199

        Diffs (updated)


        src/contrib/loggraph/src/java/org/apache/zookeeper/graph/TxnLogSource.java 809c455
        src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40
        src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 9216751
        src/java/main/org/apache/zookeeper/MultiResponse.java 97d4c7d
        src/java/main/org/apache/zookeeper/MultiTransactionRecord.java af3b58d
        src/java/main/org/apache/zookeeper/Op.java 3c3db2e
        src/java/main/org/apache/zookeeper/OpResult.java 514318f
        src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20
        src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0
        src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e
        src/java/main/org/apache/zookeeper/server/DataTree.java 757a572
        src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 336827a
        src/java/main/org/apache/zookeeper/server/LogFormatter.java 9be3fe3
        src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e44c65e
        src/java/main/org/apache/zookeeper/server/Request.java c6a2249
        src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b
        src/java/main/org/apache/zookeeper/server/TraceFormatter.java 60d1cc7
        src/java/main/org/apache/zookeeper/server/ZKDatabase.java 2842803
        src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 9ccaa0e
        src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java 938cf19
        src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 2f77e9d
        src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java a1c8ce2
        src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java 27bf496
        src/java/main/org/apache/zookeeper/server/quorum/Learner.java a97a543
        src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java c518792
        src/java/main/org/apache/zookeeper/server/quorum/LearnerSyncRequest.java bfbc9a8
        src/java/main/org/apache/zookeeper/server/quorum/Observer.java 8e77ac8
        src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java e94414f
        src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java 82d1468
        src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java f9647c4
        src/java/test/org/apache/zookeeper/common/OpCodeTest.java PRE-CREATION
        src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java e37cbec
        src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 7a5a75b
        src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java a134210

        Diff: https://reviews.apache.org/r/2015/diff

        Testing
        -------

        Thanks,

        Thomas

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2015/ ----------------------------------------------------------- (Updated 2011-10-24 17:44:19.879262) Review request for zookeeper. Summary ------- There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in? I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there? This addresses bug ZOOKEEPER-1199 . https://issues.apache.org/jira/browse/ZOOKEEPER-1199 Diffs (updated) src/contrib/loggraph/src/java/org/apache/zookeeper/graph/TxnLogSource.java 809c455 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 9216751 src/java/main/org/apache/zookeeper/MultiResponse.java 97d4c7d src/java/main/org/apache/zookeeper/MultiTransactionRecord.java af3b58d src/java/main/org/apache/zookeeper/Op.java 3c3db2e src/java/main/org/apache/zookeeper/OpResult.java 514318f src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20 src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/main/org/apache/zookeeper/server/DataTree.java 757a572 src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 336827a src/java/main/org/apache/zookeeper/server/LogFormatter.java 9be3fe3 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e44c65e src/java/main/org/apache/zookeeper/server/Request.java c6a2249 src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b src/java/main/org/apache/zookeeper/server/TraceFormatter.java 60d1cc7 src/java/main/org/apache/zookeeper/server/ZKDatabase.java 2842803 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 9ccaa0e src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java 938cf19 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 2f77e9d src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java a1c8ce2 src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java 27bf496 src/java/main/org/apache/zookeeper/server/quorum/Learner.java a97a543 src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java c518792 src/java/main/org/apache/zookeeper/server/quorum/LearnerSyncRequest.java bfbc9a8 src/java/main/org/apache/zookeeper/server/quorum/Observer.java 8e77ac8 src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java e94414f src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java 82d1468 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java f9647c4 src/java/test/org/apache/zookeeper/common/OpCodeTest.java PRE-CREATION src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java e37cbec src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 7a5a75b src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java a134210 Diff: https://reviews.apache.org/r/2015/diff Testing ------- Thanks, Thomas
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

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

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

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2015/
        -----------------------------------------------------------

        (Updated 2011-10-25 16:51:47.887909)

        Review request for zookeeper.

        Changes
        -------

        centralized the logic to deserialize requests in PrepRequestProcessor and FinalRequestProcessor

        Summary
        -------

        There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in?
        I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there?

        This addresses bug ZOOKEEPER-1199.
        https://issues.apache.org/jira/browse/ZOOKEEPER-1199

        Diffs (updated)


        src/contrib/loggraph/src/java/org/apache/zookeeper/graph/TxnLogSource.java 809c455
        src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40
        src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 9216751
        src/java/main/org/apache/zookeeper/MultiResponse.java 97d4c7d
        src/java/main/org/apache/zookeeper/MultiTransactionRecord.java af3b58d
        src/java/main/org/apache/zookeeper/Op.java 3c3db2e
        src/java/main/org/apache/zookeeper/OpResult.java 514318f
        src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20
        src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0
        src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e
        src/java/main/org/apache/zookeeper/server/DataTree.java 757a572
        src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 336827a
        src/java/main/org/apache/zookeeper/server/LogFormatter.java 9be3fe3
        src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e44c65e
        src/java/main/org/apache/zookeeper/server/Request.java c6a2249
        src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b
        src/java/main/org/apache/zookeeper/server/TraceFormatter.java 60d1cc7
        src/java/main/org/apache/zookeeper/server/ZKDatabase.java 2842803
        src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 9ccaa0e
        src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java 938cf19
        src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 2f77e9d
        src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java a1c8ce2
        src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java 27bf496
        src/java/main/org/apache/zookeeper/server/quorum/Learner.java a97a543
        src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java 2d0714f
        src/java/main/org/apache/zookeeper/server/quorum/LearnerSyncRequest.java bfbc9a8
        src/java/main/org/apache/zookeeper/server/quorum/Observer.java 8e77ac8
        src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java e94414f
        src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java 82d1468
        src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java f9647c4
        src/java/test/org/apache/zookeeper/common/OpCodeTest.java PRE-CREATION
        src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java e37cbec
        src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 38a3c57
        src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java a134210

        Diff: https://reviews.apache.org/r/2015/diff

        Testing
        -------

        Thanks,

        Thomas

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2015/ ----------------------------------------------------------- (Updated 2011-10-25 16:51:47.887909) Review request for zookeeper. Changes ------- centralized the logic to deserialize requests in PrepRequestProcessor and FinalRequestProcessor Summary ------- There are four places in the ZK code, that mapped OpCodes to string representations. One of them wasn't used anymore, two others represented the same mapping and the last one provided four letter representations. As you can see in the definition of the OpCode enum, there were strings missing for some newer added OpCodes. You might want to say, which strings I should put in? I spotted an inconsistency in server/quorum/ReadOnlyRequestProcessor.java. There's a switch checking for a write operation which does not include multi, start- and stopsession. What is intended there? This addresses bug ZOOKEEPER-1199 . https://issues.apache.org/jira/browse/ZOOKEEPER-1199 Diffs (updated) src/contrib/loggraph/src/java/org/apache/zookeeper/graph/TxnLogSource.java 809c455 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 9216751 src/java/main/org/apache/zookeeper/MultiResponse.java 97d4c7d src/java/main/org/apache/zookeeper/MultiTransactionRecord.java af3b58d src/java/main/org/apache/zookeeper/Op.java 3c3db2e src/java/main/org/apache/zookeeper/OpResult.java 514318f src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20 src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/main/org/apache/zookeeper/server/DataTree.java 757a572 src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 336827a src/java/main/org/apache/zookeeper/server/LogFormatter.java 9be3fe3 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e44c65e src/java/main/org/apache/zookeeper/server/Request.java c6a2249 src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b src/java/main/org/apache/zookeeper/server/TraceFormatter.java 60d1cc7 src/java/main/org/apache/zookeeper/server/ZKDatabase.java 2842803 src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 9ccaa0e src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java 938cf19 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 2f77e9d src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java a1c8ce2 src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java 27bf496 src/java/main/org/apache/zookeeper/server/quorum/Learner.java a97a543 src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java 2d0714f src/java/main/org/apache/zookeeper/server/quorum/LearnerSyncRequest.java bfbc9a8 src/java/main/org/apache/zookeeper/server/quorum/Observer.java 8e77ac8 src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java e94414f src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java 82d1468 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java f9647c4 src/java/test/org/apache/zookeeper/common/OpCodeTest.java PRE-CREATION src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java e37cbec src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java 38a3c57 src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java a134210 Diff: https://reviews.apache.org/r/2015/diff Testing ------- Thanks, Thomas
        Hide
        Patrick Hunt added a comment -

        Thomas can you summarize the mailing list discussion so far for this jira.

        I haven't tracked this closely, but is there a way to update the code generation to allow OpCode types to be passed in/out? (perhaps a super?) Thereby mitigating Ben's concerns I would think.

        Show
        Patrick Hunt added a comment - Thomas can you summarize the mailing list discussion so far for this jira. I haven't tracked this closely, but is there a way to update the code generation to allow OpCode types to be passed in/out? (perhaps a super?) Thereby mitigating Ben's concerns I would think.

          People

          • Assignee:
            Thomas Koch
            Reporter:
            Thomas Koch
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development