Hadoop Common
  1. Hadoop Common
  2. HADOOP-6578

Configuration should trim whitespace around a lot of value types

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.22.0, 0.23.0
    • Fix Version/s: 0.22.0, 0.23.0
    • Component/s: conf
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I've seen multiple users make an error where they've listed some whitespace around a class name (eg for configuring a scheduler). This results in a ClassNotFoundException which is very hard to debug, as you don't notice the whitespace in the exception! We should simply trim the whitespace in Configuration.getClass and Configuration.getClasses to avoid this class of user error.

      Similarly, we should trim in getInt, getLong, etc - anywhere that whitespace doesn't have semantic meaning we should be a little less strict on input.

      1. HADOOP-6578.patch
        8 kB
        Michele Catasta

        Issue Links

          Activity

          Hide
          Michele Catasta added a comment -

          Attaching a trivial patch that takes care only of getClass* methods.

          I took a look at all the getters in Configuration, and I think we can safely trim property name also for:
          getBoolean(), getFloat(), getInt(), getLong(). Let me know if I should extend the patch to those (and even other) methods.

          As per HADOOP-6133, I didn't add any test because: "No additional tests required - this code path is exercised widely by all parts of Hadoop".

          Show
          Michele Catasta added a comment - Attaching a trivial patch that takes care only of getClass* methods. I took a look at all the getters in Configuration, and I think we can safely trim property name also for: getBoolean(), getFloat(), getInt(), getLong(). Let me know if I should extend the patch to those (and even other) methods. As per HADOOP-6133 , I didn't add any test because: "No additional tests required - this code path is exercised widely by all parts of Hadoop".
          Hide
          Michele Catasta added a comment -

          Fixed also the getters mentioned in the previous comment.
          Slight refactoring - now in tune with getStrings() vs getTrimmedStrings().

          Show
          Michele Catasta added a comment - Fixed also the getters mentioned in the previous comment. Slight refactoring - now in tune with getStrings() vs getTrimmedStrings().
          Hide
          Todd Lipcon added a comment -
          • In getTrimmed(String), why not have this function just wrap the existing get() and trim the result, rather than duplicating its functionality?
          • I'm not sure it makes sense to modify getClassByName. For external users calling this function directly, I don't think we should trim for them. For internal users, you've already covered those cases in .getClass() and .getClasses().
          • Since it's easy to test, might as well throw a few more test cases in the existing TestConfiguration.
          Show
          Todd Lipcon added a comment - In getTrimmed(String), why not have this function just wrap the existing get() and trim the result, rather than duplicating its functionality? I'm not sure it makes sense to modify getClassByName. For external users calling this function directly, I don't think we should trim for them. For internal users, you've already covered those cases in .getClass() and .getClasses(). Since it's easy to test, might as well throw a few more test cases in the existing TestConfiguration.
          Hide
          Michele Catasta added a comment -
          • done
          • reverted to trunk
          • added some tests - they should cover all the modified methods
          Show
          Michele Catasta added a comment - done reverted to trunk added some tests - they should cover all the modified methods
          Hide
          Todd Lipcon added a comment -

          Patch looks good for me. Submitting patch for Hudson to check

          Show
          Todd Lipcon added a comment - Patch looks good for me. Submitting patch for Hudson to check
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12438282/HADOOP-6578.patch
          against trunk revision 921980.

          +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 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: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/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/12438282/HADOOP-6578.patch against trunk revision 921980. +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 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: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/417/console This message is automatically generated.
          Hide
          Michele Catasta added a comment -

          Tests were failing because of a missing null check in getTrimmed(). Attached a new patch and re-submitted to Hudson.

          Show
          Michele Catasta added a comment - Tests were failing because of a missing null check in getTrimmed(). Attached a new patch and re-submitted to Hudson.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12438609/HADOOP-6578.patch
          against trunk revision 921980.

          +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 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: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/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/12438609/HADOOP-6578.patch against trunk revision 921980. +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 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: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/418/console This message is automatically generated.
          Hide
          Michele Catasta added a comment -

          Only one test failed in the previous Hudson build, and it looks unrelated.
          I run the full test suite in local with the patch applied to trunk and all tests passed.

          Show
          Michele Catasta added a comment - Only one test failed in the previous Hudson build, and it looks unrelated. I run the full test suite in local with the patch applied to trunk and all tests passed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12438609/HADOOP-6578.patch
          against trunk revision 1031422.

          +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 appears to have generated 1 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 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.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/61//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/61//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/61//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/12438609/HADOOP-6578.patch against trunk revision 1031422. +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 appears to have generated 1 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 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. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/61//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/61//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/61//console This message is automatically generated.
          Hide
          Michele Catasta added a comment -

          Javadoc warnings are coming from the hadoop.security package.
          ORing with the previous Hudson run in March, it should be a +1 overall.

          Show
          Michele Catasta added a comment - Javadoc warnings are coming from the hadoop.security package. ORing with the previous Hudson run in March, it should be a +1 overall.
          Hide
          Eli Collins added a comment -

          +1 lgtm

          Show
          Eli Collins added a comment - +1 lgtm
          Hide
          Eli Collins added a comment -

          I've committed this to trunk and merged to 22. Thanks Michele!

          Show
          Eli Collins added a comment - I've committed this to trunk and merged to 22. Thanks Michele!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #567 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/567/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #567 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/567/ )

            People

            • Assignee:
              Michele Catasta
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development