Hadoop Common
  1. Hadoop Common
  2. HADOOP-7377

Fix command name handling affecting DFSAdmin

    Details

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

      Description

      When an error occurs in the get/set quota commands in DFSAdmin, they are displaying the following:
      setQuota: failed to get SetQuotaCommand.NAME

      The Command class expects the NAME field to be accessible, but for DFSAdmin, it's not.

        Activity

        Hide
        Daryn Sharp added a comment -

        Quick cheap way to get the NAME field despite the visibility of the class and/or field.

        Show
        Daryn Sharp added a comment - Quick cheap way to get the NAME field despite the visibility of the class and/or field.
        Hide
        Daryn Sharp added a comment -

        No tests included because it fixes tests in hdfs.

        Show
        Daryn Sharp added a comment - No tests included because it fixes tests in hdfs.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/610//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/610//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/610//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/12482092/HADOOP-7377.patch against trunk revision 1134218. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/610//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/610//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/610//console This message is automatically generated.
        Hide
        Matt Foley added a comment -

        I'm uncomfortable using reflection with access permission override, to get a datum that seems like it could be fetched using provided methods such as getCommandName(). However, Daryn has convinced me that it would require a massive change of legacy code to make this better:

        [Daryn] The CommandFactory is supposed to call setName(...) on the instance it creates, using the name fed to the factory (since commands can have more than one invokable name). setName()/getName() should be just using the instance field name. The static NAME is legacy and was not intended to remain. But Count and the DFSAdmin commands, unfortunately, use it. I intend to kill both getCommandName() and NAME, if possible, after DFSAdmin is adapted to better use the framework. Basically, the command should not know it's name, that's the factory's job, and the factory tells the command how it was conjured up. But in the meantime, NAME is needed.

        So, +1 on this patch.

        Auto-test passes except for new test, but this patch will be tested by existing DFSAdmin unit tests starting to work.
        I'll commit this in 24 hours to allow for other comments.

        Daryn, please list the exact unit tests that will be fixed by this submission. Thanks.

        Show
        Matt Foley added a comment - I'm uncomfortable using reflection with access permission override, to get a datum that seems like it could be fetched using provided methods such as getCommandName(). However, Daryn has convinced me that it would require a massive change of legacy code to make this better: [Daryn] The CommandFactory is supposed to call setName(...) on the instance it creates, using the name fed to the factory (since commands can have more than one invokable name). setName()/getName() should be just using the instance field name. The static NAME is legacy and was not intended to remain. But Count and the DFSAdmin commands, unfortunately, use it. I intend to kill both getCommandName() and NAME, if possible, after DFSAdmin is adapted to better use the framework. Basically, the command should not know it's name, that's the factory's job, and the factory tells the command how it was conjured up. But in the meantime, NAME is needed. So, +1 on this patch. Auto-test passes except for new test, but this patch will be tested by existing DFSAdmin unit tests starting to work. I'll commit this in 24 hours to allow for other comments. Daryn, please list the exact unit tests that will be fixed by this submission. Thanks.
        Hide
        Daryn Sharp added a comment -

        This patch fixes all of the failing negative tests for DFSAdmin in TestHDFSCLI. As of build #777, the negative DFSAdmin tests are the only tests that are failing in TestHDFSCLI.

        Show
        Daryn Sharp added a comment - This patch fixes all of the failing negative tests for DFSAdmin in TestHDFSCLI . As of build #777, the negative DFSAdmin tests are the only tests that are failing in TestHDFSCLI .
        Hide
        Matt Foley added a comment -

        This patch needs to be rebased for trunk.

        Show
        Matt Foley added a comment - This patch needs to be rebased for trunk.
        Hide
        Matt Foley added a comment -

        No it doesn't need to be rebased.

        Committed to trunk. Thanks, Daryn!

        Show
        Matt Foley added a comment - No it doesn't need to be rebased. Committed to trunk. Thanks, Daryn!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #657 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/657/)
        HADOOP-7377. Fix command name handling affecting DFSAdmin. Contributed by Daryn Sharp.

        mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1136223
        Files :

        • /hadoop/common/trunk/common/src/java/org/apache/hadoop/fs/shell/Command.java
        • /hadoop/common/trunk/common/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #657 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/657/ ) HADOOP-7377 . Fix command name handling affecting DFSAdmin. Contributed by Daryn Sharp. mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1136223 Files : /hadoop/common/trunk/common/src/java/org/apache/hadoop/fs/shell/Command.java /hadoop/common/trunk/common/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-maven #43 (See https://builds.apache.org/job/Hadoop-Common-trunk-maven/43/)

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

          People

          • Assignee:
            Daryn Sharp
            Reporter:
            Daryn Sharp
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development