Hadoop Common
  1. Hadoop Common
  2. HADOOP-6227

Configuration does not lock parameters marked final if they have no value.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: conf
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      A use-case: Recently, we stumbled on a bug that wanted us to disable the feature to run a debug script in map/reduce on tasks that fail, specified by mapred.

      {map|reduce}

      .task.debug.script. The best way of disabling it seemed to be to set it with no value in the cluster configuration and mark it final. This did not work, however, because the code in configuration explicitly checks if the value is not null before adding it to the list of final parameters.

      Without an ability to do this, there might be a need to explicitly have a special way of disabling optional features.

      1. patch-6227.txt
        3 kB
        Amareshwari Sriramadasu
      2. patch-6227-0.20.txt
        2 kB
        Amareshwari Sriramadasu
      3. patch-6227-ydist.txt
        3 kB
        Vinod Kumar Vavilapalli

        Activity

        Hide
        Robert Chansler added a comment -

        Editorial pass over all release notes prior to publication of 0.21. Bug.

        Show
        Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21. Bug.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk #82 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/82/)
        . Fix Configuration to allow final parameters to be set to null and prevent them from being overridden. Contributed by Amareshwari Sriramadasu.

        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk #82 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/82/ ) . Fix Configuration to allow final parameters to be set to null and prevent them from being overridden. Contributed by Amareshwari Sriramadasu.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #4 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/4/)
        . Fix Configuration to allow final parameters to be set to null and prevent them from being overridden. Contributed by Amareshwari Sriramadasu.

        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #4 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/4/ ) . Fix Configuration to allow final parameters to be set to null and prevent them from being overridden. Contributed by Amareshwari Sriramadasu.
        Hide
        Hemanth Yamijala added a comment -

        I just committed this to trunk. Thanks, Amareshwari !

        I did not commit to the 0.20 branch, because there's an active vote in progress and did not think this existing bug should block the release. If decided otherwise, we can commit to the 0.20 branch as well.

        Show
        Hemanth Yamijala added a comment - I just committed this to trunk. Thanks, Amareshwari ! I did not commit to the 0.20 branch, because there's an active vote in progress and did not think this existing bug should block the release. If decided otherwise, we can commit to the 0.20 branch as well.
        Hide
        Arun C Murthy added a comment -

        +1

        Show
        Arun C Murthy added a comment - +1
        Hide
        Vinod Kumar Vavilapalli added a comment -

        I just tested this fix on a single node cluster so as to enable/disable mapred debug script by setting it to null. The patch works as expected. A null value marked as final on server side can no longer be overridden by clients.

        Show
        Vinod Kumar Vavilapalli added a comment - I just tested this fix on a single node cluster so as to enable/disable mapred debug script by setting it to null. The patch works as expected. A null value marked as final on server side can no longer be overridden by clients.
        Hide
        Hemanth Yamijala added a comment -

        Code changes look fine to me. +1.

        Show
        Hemanth Yamijala added a comment - Code changes look fine to me. +1.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Yahoo! distribution patch. The patch for branch 0.20 will not apply to this distribution with HADOOP-6184 already in.

        Show
        Vinod Kumar Vavilapalli added a comment - Yahoo! distribution patch. The patch for branch 0.20 will not apply to this distribution with HADOOP-6184 already in.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12418238/patch-6227.txt
        against trunk revision 809491.

        +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 passed 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/4/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/4/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/4/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/4/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/12418238/patch-6227.txt against trunk revision 809491. +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 passed 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/4/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/4/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/4/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/4/console This message is automatically generated.
        Hide
        Amareshwari Sriramadasu added a comment -

        Patch with the fix

        Show
        Amareshwari Sriramadasu added a comment - Patch with the fix
        Hide
        Hemanth Yamijala added a comment -

        The code in configuration looks like this:

                // Ignore this parameter if it has already been marked as 'final'
                if (attr != null && value != null) {
                  if (!finalParameters.contains(attr)) {
                    properties.setProperty(attr, value);
                    if (storeResource) {
                      updatingResource.put(attr, name.toString());
                    }
                    if (finalParameter)
                      finalParameters.add(attr);
                  } else {
                    LOG.warn(name+":a attempt to override final parameter: "+attr
                             +";  Ignoring.");
                  }
                }
        

        While the check for value being null is required before adding the property to the 'properties' instance, adding it to finalParameters or for that matter even updatingResource shouldn't be an issue even if the value is null. This would certainly fix the current issue. Thoughts ?

        Show
        Hemanth Yamijala added a comment - The code in configuration looks like this: // Ignore this parameter if it has already been marked as ' final ' if (attr != null && value != null ) { if (!finalParameters.contains(attr)) { properties.setProperty(attr, value); if (storeResource) { updatingResource.put(attr, name.toString()); } if (finalParameter) finalParameters.add(attr); } else { LOG.warn(name+ ":a attempt to override final parameter: " +attr + "; Ignoring." ); } } While the check for value being null is required before adding the property to the 'properties' instance, adding it to finalParameters or for that matter even updatingResource shouldn't be an issue even if the value is null. This would certainly fix the current issue. Thoughts ?

          People

          • Assignee:
            Amareshwari Sriramadasu
            Reporter:
            Hemanth Yamijala
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development