Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1887

MRAsyncDiskService does not properly absolutize volume root paths

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      MAPREDUCE-1887. MRAsyncDiskService now properly absolutizes volume root paths. (Aaron Kimball via zshao)
      Show
      MAPREDUCE-1887 . MRAsyncDiskService now properly absolutizes volume root paths. (Aaron Kimball via zshao)

      Description

      In MRAsyncDiskService, volume names are sometimes specified as relative paths, which are not converted to absolute paths. This can cause errors of the form "cannot delete </full/path/to/foo> since it is outside of <relative/volume/root>" even though the actual path is inside the root.

      1. MAPREDUCE-1887.patch
        8 kB
        Aaron Kimball
      2. MAPREDUCE-1887.3.patch
        8 kB
        Aaron Kimball
      3. MAPREDUCE-1887.2.patch
        8 kB
        Aaron Kimball

        Activity

        Hide
        Aaron Kimball added a comment -

        This has caused intermittent failure of TestMRServerPorts on our hudson instance. I am attaching a patch with a test case that reliably reproduces this error as well as the fix.

        Show
        Aaron Kimball added a comment - This has caused intermittent failure of TestMRServerPorts on our hudson instance. I am attaching a patch with a test case that reliably reproduces this error as well as the fix.
        Hide
        Todd Lipcon added a comment -

        Couple notes:

        • Rather than changing all the instances of "volumes" to "this.volumes" can you rename one of the variables? like rename the constructor argument to unnormalizedVolumes, and then you can just use "volumes" throughout? That should reduce changes that someone changing this code later makes the mistake again.
        • Missing @Test annotation on testRelativeToWorking()
        • Might be worth adding a comment to testVolumeNormalization stating it's a regression test for this JIRA number.
        • Either remove or change the log message "Volume: foo -> bar" - eg "Normalized MRAsyncDiskService volume foo -> bar" or just get rid of it.
        Show
        Todd Lipcon added a comment - Couple notes: Rather than changing all the instances of "volumes" to "this.volumes" can you rename one of the variables? like rename the constructor argument to unnormalizedVolumes, and then you can just use "volumes" throughout? That should reduce changes that someone changing this code later makes the mistake again. Missing @Test annotation on testRelativeToWorking() Might be worth adding a comment to testVolumeNormalization stating it's a regression test for this JIRA number. Either remove or change the log message "Volume: foo -> bar" - eg "Normalized MRAsyncDiskService volume foo -> bar" or just get rid of it.
        Hide
        Aaron Kimball added a comment -

        new patch to address Todd's comments.

        Show
        Aaron Kimball added a comment - new patch to address Todd's comments.
        Hide
        Todd Lipcon added a comment -

        lgtm. +1

        Show
        Todd Lipcon added a comment - lgtm. +1
        Hide
        Zheng Shao added a comment -

        Code looks good.

        Can we change

        +   * @param nonCanonicalVols The roots of the file system volumes, which may not
        +   * be canonical paths.
        

        to

        +   * @param nonCanonicalVols The roots of the file system volumes, which can be 
        +   * absolute paths from root or relative path from cwd.
        

        ?

        I think the second one is easier to understand.

        Show
        Zheng Shao added a comment - Code looks good. Can we change + * @param nonCanonicalVols The roots of the file system volumes, which may not + * be canonical paths. to + * @param nonCanonicalVols The roots of the file system volumes, which can be + * absolute paths from root or relative path from cwd. ? I think the second one is easier to understand.
        Hide
        Aaron Kimball added a comment -

        Agree that the javadoc comment was confusing. Updated. Thanks for the fast review!

        Show
        Aaron Kimball added a comment - Agree that the javadoc comment was confusing. Updated. Thanks for the fast review!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12447778/MAPREDUCE-1887.3.patch
        against trunk revision 957086.

        +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 failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/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/12447778/MAPREDUCE-1887.3.patch against trunk revision 957086. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/260/console This message is automatically generated.
        Hide
        Ravi Gummadi added a comment -

        LOG.debug() statement is not in LOG.isDebugEnabled() check and causes unnecessary string concatenations in the for loop. Would you please add if(LOG.isDebugEnabled()) { } around that ?

        Show
        Ravi Gummadi added a comment - LOG.debug() statement is not in LOG.isDebugEnabled() check and causes unnecessary string concatenations in the for loop. Would you please add if(LOG.isDebugEnabled()) { } around that ?
        Hide
        Todd Lipcon added a comment -

        Ravi: this log message only runs once at tasktracker startup, and the number of volumes is usually <12. Not worth guarding in isDebugEnabled since it's not going to ever be a bottleneck.

        Show
        Todd Lipcon added a comment - Ravi: this log message only runs once at tasktracker startup, and the number of volumes is usually <12. Not worth guarding in isDebugEnabled since it's not going to ever be a bottleneck.
        Hide
        Ravi Gummadi added a comment -

        Yes.
        Fine with me even if it is not modified as it is not a big perf issue.

        Show
        Ravi Gummadi added a comment - Yes. Fine with me even if it is not modified as it is not a big perf issue.
        Hide
        Zheng Shao added a comment -

        Aaron, can you take a look at the unit test failures?

        Show
        Zheng Shao added a comment - Aaron, can you take a look at the unit test failures?
        Hide
        Aaron Kimball added a comment -

        Unfortunately, Hudson's web server is not responding to me, so not at present. I've been trying every couple hours but no luck yet.

        Show
        Aaron Kimball added a comment - Unfortunately, Hudson's web server is not responding to me, so not at present. I've been trying every couple hours but no luck yet.
        Hide
        Aaron Kimball added a comment -

        Trying again with Hudson, to get this result more accessible.

        Show
        Aaron Kimball added a comment - Trying again with Hudson, to get this result more accessible.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12447778/MAPREDUCE-1887.3.patch
        against trunk revision 957437.

        +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 failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/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/12447778/MAPREDUCE-1887.3.patch against trunk revision 957437. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/585/console This message is automatically generated.
        Hide
        Zheng Shao added a comment -

        Can you take a look at the failed contrib tests?

        Show
        Zheng Shao added a comment - Can you take a look at the failed contrib tests?
        Hide
        Aaron Kimball added a comment -

        The only failing test has failed for the last 37 builds. Unrelated to this patch. I think we're good.

        Show
        Aaron Kimball added a comment - The only failing test has failed for the last 37 builds. Unrelated to this patch. I think we're good.
        Hide
        Zheng Shao added a comment -

        Committed revision 957772. Thanks Aaron!

        Show
        Zheng Shao added a comment - Committed revision 957772. Thanks Aaron!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #523 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/523/)

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

          People

          • Assignee:
            Aaron Kimball
            Reporter:
            Aaron Kimball
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development