Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.22.0
    • Component/s: io, native
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      MapReduce is currently using a race-prone workaround to approximate chmod() because forking chmod is too expensive. This race is causing build failures (and probably task failures too). We should implement chmod in the NativeIO library so we can have good performance (ie not fork) and still not be prone to races.

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Although it would be useful to add a bunch of other JNI wrappers (eg chgrp, stat, readlink) I'd like to just do chmod in this JIRA. The reasoning is that chmod in particular is necessary to fix the MR build, so this needs to go into 0.22, whereas the others are truly improvements.

          If others disagree, happy to write the wrappers.

          Show
          Todd Lipcon added a comment - Although it would be useful to add a bunch of other JNI wrappers (eg chgrp, stat, readlink) I'd like to just do chmod in this JIRA. The reasoning is that chmod in particular is necessary to fix the MR build, so this needs to go into 0.22, whereas the others are truly improvements. If others disagree, happy to write the wrappers.
          Hide
          Eli Collins added a comment -

          +1 looks great

          Show
          Eli Collins added a comment - +1 looks great
          Hide
          Todd Lipcon added a comment -

          Ran tests with compile.native=1 and without compile.native, both passed (except for HADOOP-7111 issues)

          I'll commit this this evening.

          Show
          Todd Lipcon added a comment - Ran tests with compile.native=1 and without compile.native, both passed (except for HADOOP-7111 issues) I'll commit this this evening.
          Hide
          Konstantin Shvachko added a comment -

          What is wrong with the current implementation and why native is better?
          Could you please explain.

          Show
          Konstantin Shvachko added a comment - What is wrong with the current implementation and why native is better? Could you please explain.
          Hide
          Todd Lipcon added a comment -

          Konstantin: depends which "current implementation" you're referring to.

          In trunk right now, RawLocalFS uses the equivalent of system("chmod ... <path>") to perform a chmod. This is very inefficient, since Java uses fork, not vfork, and thus incurs a cost to set up new vm_area structures for the child process, etc. As I imagine you've seen before, if the process has a very large heap (many GB) this can take several milliseconds.

          HADOOP-6304 identifies this issue (in fact calls it an "emergency bug fix") and proposes using the Java setReadable|setWritable|setExecutable APIs instead. These don't have the cost of a fork, but have two problems:
          1) They have limited power since in typical java fashion are a least-common-denominator API. That is to say, they don't support setting a file to arbitrary modes, have no concept of "group" ownership, etc. If you'll look at the implementation in that JIRA you'll see what I mean.
          2) Because the APIs are awkward, the only way to get reasonably close to full chmod support is to first chmod a file down to 000 and then work back up to the correct permissions. This is race prone and has been causing build failures for several months (see MAPREDUCE-2238)

          So, this JIRA adds the JNI call which fixes all three issues issues. It has the full ability to chmod to any mode, is atomic, and is very efficient.

          Does that answer the question?

          Show
          Todd Lipcon added a comment - Konstantin: depends which "current implementation" you're referring to. In trunk right now, RawLocalFS uses the equivalent of system("chmod ... <path>") to perform a chmod. This is very inefficient, since Java uses fork, not vfork, and thus incurs a cost to set up new vm_area structures for the child process, etc. As I imagine you've seen before, if the process has a very large heap (many GB) this can take several milliseconds. HADOOP-6304 identifies this issue (in fact calls it an "emergency bug fix") and proposes using the Java setReadable|setWritable|setExecutable APIs instead. These don't have the cost of a fork, but have two problems: 1) They have limited power since in typical java fashion are a least-common-denominator API. That is to say, they don't support setting a file to arbitrary modes, have no concept of "group" ownership, etc. If you'll look at the implementation in that JIRA you'll see what I mean. 2) Because the APIs are awkward, the only way to get reasonably close to full chmod support is to first chmod a file down to 000 and then work back up to the correct permissions. This is race prone and has been causing build failures for several months (see MAPREDUCE-2238 ) So, this JIRA adds the JNI call which fixes all three issues issues. It has the full ability to chmod to any mode, is atomic, and is very efficient. Does that answer the question?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12468660/hadoop-7110.txt
          against trunk revision 1060632.

          +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 (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.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/185//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/185//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/185//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/12468660/hadoop-7110.txt against trunk revision 1060632. +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 (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. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/185//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/185//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/185//console This message is automatically generated.
          Hide
          Konstantin Shvachko added a comment -

          Thanks for explanation, Todd. I understand now. As I see it the problem you observed in MAPREDUCE-2238 is caused by that Localizer.PermissionsHandler first clears the permissions by setting them to 000, then setting the ones passed. So if it fails right after setting them to 000 it will be hard to remove such file or directory.
          So why don't we just set permissions to the values specified (in the right order, which is probably important) rather than introducing more JNI code. Nobody said we should support posix permissions.
          Your patch is an alternative implementation of HADOOP-6304. You should have attached it to that jira rather than creating the new one. Now the discussion is split into parts, making it hard to follow.

          Show
          Konstantin Shvachko added a comment - Thanks for explanation, Todd. I understand now. As I see it the problem you observed in MAPREDUCE-2238 is caused by that Localizer.PermissionsHandler first clears the permissions by setting them to 000, then setting the ones passed. So if it fails right after setting them to 000 it will be hard to remove such file or directory. So why don't we just set permissions to the values specified (in the right order, which is probably important) rather than introducing more JNI code. Nobody said we should support posix permissions. Your patch is an alternative implementation of HADOOP-6304 . You should have attached it to that jira rather than creating the new one. Now the discussion is split into parts, making it hard to follow.
          Hide
          Todd Lipcon added a comment -

          So if it fails right after setting them to 000 it will be hard to remove such file or directory

          It's not due to failure of the process doing the chmod. It's due to other processes or threads working inside the directory failing because there is a moment in which they can't traverse the path inside the directory. As we've seen in MR-2238 this race really does happen - we've got build failures on three separate environments (Apache Hudson, my Hudson, and Greg's build box) to prove it.

          So why don't we just set permissions to the values specified (in the right order, which is probably important) rather than introducing more JNI code

          The API is not flexible enough to do this. If you have a file 755 and want it to be 700, the only way to get rid of the group/other bits is to first clear those bits for all, then add them back for user only.

          Nobody said we should support posix permissions

          Since we now support security, we have to be able to make some files mode 700 and others 755, etc. There's no way around it. The question here is implementation method, not whether we should support an API since we've had since 2008. Removing the ability to chmod isn't an option, considering compatibility.

          Your patch is an alternative implementation of HADOOP-6304

          And HADOOP-6304 is a sort-of-copy-paste of code originally introduced in MAPREDUCE-842. I filed this JIRA as part of the fix for MAPREDUCE-2238 (which is caused by the MR code that has the same problems)

          Do you have a particular issue with the JNI code?

          Show
          Todd Lipcon added a comment - So if it fails right after setting them to 000 it will be hard to remove such file or directory It's not due to failure of the process doing the chmod. It's due to other processes or threads working inside the directory failing because there is a moment in which they can't traverse the path inside the directory. As we've seen in MR-2238 this race really does happen - we've got build failures on three separate environments (Apache Hudson, my Hudson, and Greg's build box) to prove it. So why don't we just set permissions to the values specified (in the right order, which is probably important) rather than introducing more JNI code The API is not flexible enough to do this. If you have a file 755 and want it to be 700, the only way to get rid of the group/other bits is to first clear those bits for all, then add them back for user only. Nobody said we should support posix permissions Since we now support security, we have to be able to make some files mode 700 and others 755, etc. There's no way around it. The question here is implementation method, not whether we should support an API since we've had since 2008. Removing the ability to chmod isn't an option, considering compatibility. Your patch is an alternative implementation of HADOOP-6304 And HADOOP-6304 is a sort-of-copy-paste of code originally introduced in MAPREDUCE-842 . I filed this JIRA as part of the fix for MAPREDUCE-2238 (which is caused by the MR code that has the same problems) Do you have a particular issue with the JNI code?
          Hide
          Konstantin Shvachko added a comment -

          JNI should be used as the last resort, imo.
          And it seems this is the case in this situation. I don't see other reasonable way implementing permissions setting.
          +1

          Show
          Konstantin Shvachko added a comment - JNI should be used as the last resort, imo. And it seems this is the case in this situation. I don't see other reasonable way implementing permissions setting. +1
          Hide
          Todd Lipcon added a comment -

          Thanks. I will commit this in the next day or two if there are no other objections.

          Show
          Todd Lipcon added a comment - Thanks. I will commit this in the next day or two if there are no other objections.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #586 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/586/)
          HADOOP-7110. Implement chmod with JNI. Contributed by Todd Lipcon

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #586 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/586/ ) HADOOP-7110 . Implement chmod with JNI. Contributed by Todd Lipcon
          Hide
          Todd Lipcon added a comment -

          Committed to trunk and branch - branch because, though this is an "optimization", without this optimization serious issues can arise (Arun called HADOOP-6304 an "emergency bug fix" which served the same purpose)

          Show
          Todd Lipcon added a comment - Committed to trunk and branch - branch because, though this is an "optimization", without this optimization serious issues can arise (Arun called HADOOP-6304 an "emergency bug fix" which served the same purpose)
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-22-branch #19 (See https://hudson.apache.org/hudson/job/Hadoop-Common-22-branch/19/)
          HADOOP-7110. Implement chmod with JNI. Contributed by Todd Lipcon

          Show
          Hudson added a comment - Integrated in Hadoop-Common-22-branch #19 (See https://hudson.apache.org/hudson/job/Hadoop-Common-22-branch/19/ ) HADOOP-7110 . Implement chmod with JNI. Contributed by Todd Lipcon
          Hide
          Hudson added a comment -

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

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

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development