Hadoop Common
  1. Hadoop Common
  2. HADOOP-7210

Chown command is not working from FSShell.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: fs
    • Labels:
      None
    • Environment:

      Linux

    • Hadoop Flags:
      Reviewed

      Description

      chown command is not invoking the setOwner on FileSystem.

      1. HADOOP-7210.patch
        7 kB
        Uma Maheswara Rao G
      2. HADOOP-7210.1.patch
        8 kB
        Uma Maheswara Rao G
      3. HADOOP-7210.2.patch
        9 kB
        Uma Maheswara Rao G

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #647 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/647/)
          HADOOP-7210. Chown command is not working from FSShell. Contributed by Uma Maheswara Rao G

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #647 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/647/ ) HADOOP-7210 . Chown command is not working from FSShell. Contributed by Uma Maheswara Rao G
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #540 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/540/)
          HADOOP-7210. Chown command is not working from FSShell. Contributed by Uma Maheswara Rao G

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #540 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/540/ ) HADOOP-7210 . Chown command is not working from FSShell. Contributed by Uma Maheswara Rao G
          Hide
          Todd Lipcon added a comment -

          Committed to trunk, thanks Uma

          Show
          Todd Lipcon added a comment - Committed to trunk, thanks Uma
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12474960/HADOOP-7210.2.patch
          against trunk revision 1087097.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 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/324//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/324//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/324//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/12474960/HADOOP-7210.2.patch against trunk revision 1087097. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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/324//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/324//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/324//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          +1, looks good to me and verified that it fixes the tests. waiting on the hudson results before commit.

          Show
          Todd Lipcon added a comment - +1, looks good to me and verified that it fixes the tests. waiting on the hudson results before commit.
          Hide
          Uma Maheswara Rao G added a comment -

          Thanks Todd for the comments,
          I have fixed the comments, please check.

          Show
          Uma Maheswara Rao G added a comment - Thanks Todd for the comments, I have fixed the comments, please check.
          Hide
          Todd Lipcon added a comment -

          Couple small comments:

          • Rather than injecting the "Extn" class into the FS Cache, could you instead set up a new scheme by setting "fs.testfs.impl" to your test class and then calling a command on testfs:///foo? Seems like a cleaner test pattern.
          • In ChgrpHandler you don't need to check for group == null anymore, right?
          Show
          Todd Lipcon added a comment - Couple small comments: Rather than injecting the "Extn" class into the FS Cache, could you instead set up a new scheme by setting "fs.testfs.impl" to your test class and then calling a command on testfs:///foo? Seems like a cleaner test pattern. In ChgrpHandler you don't need to check for group == null anymore, right?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12474892/HADOOP-7210.1.patch
          against trunk revision 1086455.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 5 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/323//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/323//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/323//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/12474892/HADOOP-7210.1.patch against trunk revision 1086455. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 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/323//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/323//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/323//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          +1 Nice work

          Show
          Daryn Sharp added a comment - +1 Nice work
          Hide
          Uma Maheswara Rao G added a comment -

          In this case ChgrpHandler need not extend the ChownHandler

          Show
          Uma Maheswara Rao G added a comment - In this case ChgrpHandler need not extend the ChownHandler
          Hide
          Daryn Sharp added a comment -

          Good idea, I think that would work too. In that case, would chgrp even need to inherit from chown?

          Show
          Daryn Sharp added a comment - Good idea, I think that would work too. In that case, would chgrp even need to inherit from chown?
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Daryn,
          Thanks for your comments.
          I agree with you. But I have one suggestion. We can directly override the run method in Chown and Chgrp directly from CmdHandler. Currently the ChgrpHandler extends the ChownHandler thereby use the same run method. This might have been done to save some lines of code. But because of this, we are providing additional constructor and additional if checks in the code.

          For example, owner details check is not applicable for chgrp command. The owner will be always null, still we will execute the owner checks for ChgrepHandler also.

            String newOwner = (owner == null || owner.equals(file.getOwner())) ?
                                  null : owner;
            ....
            ....
            if (newOwner != null || newGroup != null) {
          

          Instead of this, Both Chown and Chgrp handlers can directly extend from CmdHandler provide the run method.

          Please tell me your suggestion on this.

          Show
          Uma Maheswara Rao G added a comment - Hi Daryn, Thanks for your comments. I agree with you. But I have one suggestion. We can directly override the run method in Chown and Chgrp directly from CmdHandler. Currently the ChgrpHandler extends the ChownHandler thereby use the same run method. This might have been done to save some lines of code. But because of this, we are providing additional constructor and additional if checks in the code. For example, owner details check is not applicable for chgrp command. The owner will be always null, still we will execute the owner checks for ChgrepHandler also. String newOwner = (owner == null || owner.equals(file.getOwner())) ? null : owner; .... .... if (newOwner != null || newGroup != null ) { Instead of this, Both Chown and Chgrp handlers can directly extend from CmdHandler provide the run method. Please tell me your suggestion on this.
          Hide
          Daryn Sharp added a comment -

          The root problem is that a prior bug that remove the FileSystem var from the ctors. This left two ctors with a String signature. Changing one of the three commands to have a ctor of (String, String) just for the purpose of working around the ambiguity doesn't seem right.

          My suggestion would be to split the user/group mode parsing into a separate parseOwnerGroup (or similar) method. Chown's ctor calls this new method with its own impl, Chgrp overrides the method with it's own impl.

          Show
          Daryn Sharp added a comment - The root problem is that a prior bug that remove the FileSystem var from the ctors. This left two ctors with a String signature. Changing one of the three commands to have a ctor of (String, String) just for the purpose of working around the ambiguity doesn't seem right. My suggestion would be to split the user/group mode parsing into a separate parseOwnerGroup (or similar) method. Chown's ctor calls this new method with its own impl, Chgrp overrides the method with it's own impl.
          Hide
          Uma Maheswara Rao G added a comment -

          Based on the passed arguments, it is creating the Handlers. when argument is -chown, it is creating ChownHandler.

          else if (cmd.equals("-chown")) {
                handler = new ChownHandler(argv[startIndex++]);
          

          But here the problem is, it is not populating owner, group values in this constructor. just it is calling supper. It is not populating the group, owner.

          protected ChownHandler(String cmd) { //for chgrp
                super(cmd);
          }
          

          Since it is not populating, setOwner will not be invoked on fileSystem.So permissions will not change.

             if (newOwner != null || newGroup != null) {
                  try {
                    srcFs.setOwner(file.getPath(), newOwner, newGroup);
          
          Show
          Uma Maheswara Rao G added a comment - Based on the passed arguments, it is creating the Handlers. when argument is -chown, it is creating ChownHandler. else if (cmd.equals( "-chown" )) { handler = new ChownHandler(argv[startIndex++]); But here the problem is, it is not populating owner, group values in this constructor. just it is calling supper. It is not populating the group, owner. protected ChownHandler( String cmd) { // for chgrp super (cmd); } Since it is not populating, setOwner will not be invoked on fileSystem.So permissions will not change. if (newOwner != null || newGroup != null ) { try { srcFs.setOwner(file.getPath(), newOwner, newGroup);

            People

            • Assignee:
              Uma Maheswara Rao G
              Reporter:
              Uma Maheswara Rao G
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development