Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.0, 3.5.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Severe code style issues.

      1. ZOOKEEPER-1201.patch
        12 kB
        Thomas Koch
      2. ZOOKEEPER-1201.patch
        12 kB
        Thomas Koch

        Issue Links

          Activity

          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for zookeeper.

          Summary
          -------

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2017/ ----------------------------------------------------------- Review request for zookeeper. Summary -------
          Hide
          Benjamin Reed added a comment -

          why is this classified as a blocker? it's really just defining constants for strings, changing an iterator, and extracting code blocks into methods right? i think the priority should be minor.

          that said, the patch looks fine to me. eugene does this look okay to you?

          Show
          Benjamin Reed added a comment - why is this classified as a blocker? it's really just defining constants for strings, changing an iterator, and extracting code blocks into methods right? i think the priority should be minor. that said, the patch looks fine to me. eugene does this look okay to you?
          Hide
          Hadoop QA added a comment -

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

          +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/581//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/581//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/581//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/12496155/ZOOKEEPER-1201.patch against trunk revision 1173949. +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/581//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/581//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/581//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/2017/#review2026
          -----------------------------------------------------------

          Ship it!

          Looks good; the code is much improved. However please note important change regarding ZOOKEEPER-1195.

          src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
          <https://reviews.apache.org/r/2017/#comment4572>

          Good point: "credentials" should be final.

          src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
          <https://reviews.apache.org/r/2017/#comment4573>

          More concise, good.

          src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
          <https://reviews.apache.org/r/2017/#comment4574>

          See https://issues.apache.org/jira/browse/ZOOKEEPER-1195 : this should be kerberosName.getHostName(), not kerberosName.getServiceName().

          • Eugene

          On 2011-09-22 18:04:41, Thomas Koch wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

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

          -----------------------------------------------------------

          (Updated 2011-09-22 18:04:41)

          Review request for zookeeper.

          Summary

          -------

          .

          This addresses bug ZOOKEEPER-1201.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1201

          Diffs

          -----

          src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java b3faa79

          Diff: https://reviews.apache.org/r/2017/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/2017/#review2026 ----------------------------------------------------------- Ship it! Looks good; the code is much improved. However please note important change regarding ZOOKEEPER-1195 . src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java < https://reviews.apache.org/r/2017/#comment4572 > Good point: "credentials" should be final. src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java < https://reviews.apache.org/r/2017/#comment4573 > More concise, good. src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java < https://reviews.apache.org/r/2017/#comment4574 > See https://issues.apache.org/jira/browse/ZOOKEEPER-1195 : this should be kerberosName.getHostName(), not kerberosName.getServiceName(). Eugene On 2011-09-22 18:04:41, Thomas Koch wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2017/ ----------------------------------------------------------- (Updated 2011-09-22 18:04:41) Review request for zookeeper. Summary ------- . This addresses bug ZOOKEEPER-1201 . https://issues.apache.org/jira/browse/ZOOKEEPER-1201 Diffs ----- src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java b3faa79 Diff: https://reviews.apache.org/r/2017/diff Testing ------- Thanks, Thomas
          Hide
          Eugene Koontz added a comment -

          Sorry; should not have checked "Ship it": it looks good except for the necessary change needed, noted in review.

          Show
          Eugene Koontz added a comment - Sorry; should not have checked "Ship it": it looks good except for the necessary change needed, noted in review.
          Hide
          Thomas Koch added a comment -

          Changed to getHostName() according to issue 1195

          Show
          Thomas Koch added a comment - Changed to getHostName() according to issue 1195
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-09-22 19:40:04.644049)

          Review request for zookeeper.

          Summary
          -------

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2017/ ----------------------------------------------------------- (Updated 2011-09-22 19:40:04.644049) Review request for zookeeper. Summary -------
          Hide
          Hadoop QA added a comment -

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

          +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/582//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/582//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/582//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/12496168/ZOOKEEPER-1201.patch against trunk revision 1173949. +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/582//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/582//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/582//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/2017/#review2029
          -----------------------------------------------------------

          Ship it!

          +1, thanks Thomas.

          • Eugene

          On 2011-09-22 19:40:04, Thomas Koch wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

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

          -----------------------------------------------------------

          (Updated 2011-09-22 19:40:04)

          Review request for zookeeper.

          Summary

          -------

          .

          This addresses bug ZOOKEEPER-1201.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1201

          Diffs

          -----

          src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java b3faa79

          Diff: https://reviews.apache.org/r/2017/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/2017/#review2029 ----------------------------------------------------------- Ship it! +1, thanks Thomas. Eugene On 2011-09-22 19:40:04, Thomas Koch wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2017/ ----------------------------------------------------------- (Updated 2011-09-22 19:40:04) Review request for zookeeper. Summary ------- . This addresses bug ZOOKEEPER-1201 . https://issues.apache.org/jira/browse/ZOOKEEPER-1201 Diffs ----- src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java b3faa79 Diff: https://reviews.apache.org/r/2017/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/12496168/ZOOKEEPER-1201.patch
          against trunk revision 1173949.

          +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/585//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/585//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/585//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/12496168/ZOOKEEPER-1201.patch against trunk revision 1173949. +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/585//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/585//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/585//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          The change looks good to me. I'd to get this into 3.4 as well. Any objections?

          Show
          Mahadev konar added a comment - The change looks good to me. I'd to get this into 3.4 as well. Any objections?
          Hide
          Mahadev konar added a comment -

          I just committed this. Thanks Thomas!

          Show
          Mahadev konar added a comment - I just committed this. Thanks Thomas!
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1318 (See https://builds.apache.org/job/ZooKeeper-trunk/1318/)
          ZOOKEEPER-1201. Clean SaslServerCallbackHandler.java. (Thomas Koch via mahadev)

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

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1318 (See https://builds.apache.org/job/ZooKeeper-trunk/1318/ ) ZOOKEEPER-1201 . Clean SaslServerCallbackHandler.java. (Thomas Koch via mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1177191 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development