Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13721

Remove stale method ViewFileSystem#getTrashCanLocation

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha2
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: viewfs
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Incompatible change
    • Release Note:
      The unused method getTrashCanLocation has been removed. This method has long been superceded by FileSystem#getTrashRoot.

      Description

      ViewFileSystem which extends FileSystem has a public method getTrashCanLocation which is neither overridden nor used by anybody. Looks like it existed when the file was created, and also I see the implementation returning homeDirectory which might not be the expected one in cases of expunge. So, inclined to remove this stale and potentially dangerous method unless anyone has any concerns.

        public Path getTrashCanLocation(final Path f) throws FileNotFoundException {
          final InodeTree.ResolveResult<FileSystem> res = 
            fsState.resolve(getUriPath(f), true);
          return res.isInternalDir() ? null : res.targetFileSystem.getHomeDirectory();
        }
      
      1. HADOOP-13721.01.patch
        1.0 kB
        Manoj Govindassamy

        Activity

        Hide
        manojg Manoj Govindassamy added a comment -
        • expunge command relies on fs.trash.classname property or the default impl TrashPolicyDefault. The default Trash policy relies on FileSystem#getTrashRoot to get the trash directory.
        • Currently I couldn't find any users for ViewFileSystem#getTrashCanLocation. Also the implementation is wrong, which return the home directory for the trash can.

        So, inclined to remove this method ViewFileSystem#getTrashCanLocation for good.

        Andrew Wang can you please let me know if I am overlooking something very obvious here.

        Show
        manojg Manoj Govindassamy added a comment - expunge command relies on fs.trash.classname property or the default impl TrashPolicyDefault . The default Trash policy relies on FileSystem#getTrashRoot to get the trash directory. Currently I couldn't find any users for ViewFileSystem#getTrashCanLocation . Also the implementation is wrong, which return the home directory for the trash can. So, inclined to remove this method ViewFileSystem#getTrashCanLocation for good. Andrew Wang can you please let me know if I am overlooking something very obvious here.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 18s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 test4tests 0m 0s 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 mvninstall 7m 27s trunk passed
        +1 compile 7m 48s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 1m 7s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 33s trunk passed
        +1 javadoc 0m 46s trunk passed
        +1 mvninstall 0m 45s the patch passed
        +1 compile 7m 28s the patch passed
        +1 javac 7m 28s the patch passed
        +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 61 unchanged - 1 fixed = 61 total (was 62)
        +1 mvnsite 0m 59s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 31s the patch passed
        +1 javadoc 0m 42s the patch passed
        +1 unit 8m 16s hadoop-common in the patch passed.
        +1 asflicense 0m 24s The patch does not generate ASF License warnings.
        41m 50s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HADOOP-13721
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12833212/HADOOP-13721.01.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux a921af304247 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 0a85d07
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10775/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10775/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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 mvninstall 7m 27s trunk passed +1 compile 7m 48s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 7s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 33s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 45s the patch passed +1 compile 7m 28s the patch passed +1 javac 7m 28s the patch passed +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 61 unchanged - 1 fixed = 61 total (was 62) +1 mvnsite 0m 59s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 31s the patch passed +1 javadoc 0m 42s the patch passed +1 unit 8m 16s hadoop-common in the patch passed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 41m 50s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13721 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12833212/HADOOP-13721.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a921af304247 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0a85d07 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10775/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10775/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        andrew.wang Andrew Wang added a comment -

        Good find Manoj. This is almost certainly supposed to be getTrashRoot and family.

        Related question though, does the default getTrashRoot implementation work for ViewFileSystem? It seems like we need to defer to the resolved filesystem's implementation, since DFS for instance re-implements it to support encryption zones.

        Show
        andrew.wang Andrew Wang added a comment - Good find Manoj. This is almost certainly supposed to be getTrashRoot and family. Related question though, does the default getTrashRoot implementation work for ViewFileSystem? It seems like we need to defer to the resolved filesystem's implementation, since DFS for instance re-implements it to support encryption zones.
        Hide
        manojg Manoj Govindassamy added a comment -

        Expunge works with ViewFileSystem.

        The way Expunge works is

            @Override
            protected void processArguments(LinkedList<PathData> args)
            throws IOException {
              FileSystem[] childFileSystems =
                  FileSystem.get(getConf()).getChildFileSystems();
              if (null != childFileSystems) {
                for (FileSystem fs : childFileSystems) {
                  Trash trash = new Trash(fs, getConf());
                  trash.expunge();
                  trash.checkpoint();
                }
              } else {
              ... 
            }
        

        Since ViewFileSystem implements getChildFileSystems() and exports all the mounted file systems, expunge is invoked on the all these mounted filesystems.

        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls /
        Found 4 items
        -r-xr-xr-x   - manoj staff          0 2016-10-14 11:30 /nn0
        -r-xr-xr-x   - manoj staff          0 2016-10-14 11:30 /nn1
        -r-xr-xr-x   - manoj staff          0 2016-10-14 11:30 /nn2
        -r-xr-xr-x   - manoj staff          0 2016-10-14 11:30 /nn3
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls /nn0
        Found 1 items
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user/manoj
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -mkdir -p /nn0/delete/test1
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -mkdir -p /nn0/delete/test2
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/delete/test1
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/delete/test2
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user/manoj
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -rm -r /nn0/delete/test1
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:32 /nn0/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/delete/test2
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/user/manoj/.Trash/Current/delete/test1
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -rm -r -skipTrash /nn0/delete/test2
        Deleted /nn0/delete/test2
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:33 /nn0/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/user/manoj/.Trash/Current/delete/test1
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -expunge
        manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:33 /nn0/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:27 /nn0/user
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj
        drwx------   - manoj supergroup          0 2016-10-14 11:34 /nn0/user/manoj/.Trash
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash/161014113451
        drwx------   - manoj supergroup          0 2016-10-14 11:32 /nn0/user/manoj/.Trash/161014113451/delete
        drwxr-xr-x   - manoj supergroup          0 2016-10-14 11:31 /nn0/user/manoj/.Trash/161014113451/delete/test1
        
        Show
        manojg Manoj Govindassamy added a comment - Expunge works with ViewFileSystem . The way Expunge works is @Override protected void processArguments(LinkedList<PathData> args) throws IOException { FileSystem[] childFileSystems = FileSystem.get(getConf()).getChildFileSystems(); if ( null != childFileSystems) { for (FileSystem fs : childFileSystems) { Trash trash = new Trash(fs, getConf()); trash.expunge(); trash.checkpoint(); } } else { ... } Since ViewFileSystem implements getChildFileSystems() and exports all the mounted file systems, expunge is invoked on the all these mounted filesystems. manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls / Found 4 items -r-xr-xr-x - manoj staff 0 2016-10-14 11:30 /nn0 -r-xr-xr-x - manoj staff 0 2016-10-14 11:30 /nn1 -r-xr-xr-x - manoj staff 0 2016-10-14 11:30 /nn2 -r-xr-xr-x - manoj staff 0 2016-10-14 11:30 /nn3 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls /nn0 Found 1 items drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user/manoj manoj@~/work/test/hadev-mg(master)*: hdfs dfs -mkdir -p /nn0/delete/test1 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -mkdir -p /nn0/delete/test2 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/delete/test1 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/delete/test2 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user/manoj manoj@~/work/test/hadev-mg(master)*: hdfs dfs -rm -r /nn0/delete/test1 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:32 /nn0/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/delete/test2 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user drwxr-xr-x - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/user/manoj/.Trash/Current/delete/test1 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -rm -r -skipTrash /nn0/delete/test2 Deleted /nn0/delete/test2 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:33 /nn0/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user drwxr-xr-x - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash/Current/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/user/manoj/.Trash/Current/delete/test1 manoj@~/work/test/hadev-mg(master)*: hdfs dfs -expunge manoj@~/work/test/hadev-mg(master)*: hdfs dfs -ls -R /nn0 drwxr-xr-x - manoj supergroup 0 2016-10-14 11:33 /nn0/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:27 /nn0/user drwxr-xr-x - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj drwx------ - manoj supergroup 0 2016-10-14 11:34 /nn0/user/manoj/.Trash drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash/161014113451 drwx------ - manoj supergroup 0 2016-10-14 11:32 /nn0/user/manoj/.Trash/161014113451/delete drwxr-xr-x - manoj supergroup 0 2016-10-14 11:31 /nn0/user/manoj/.Trash/161014113451/delete/test1
        Hide
        andrew.wang Andrew Wang added a comment -

        Gotcha. Looking at the Shell code, it resolves the child filesystem before creating the Trash, and calls getTrashRoot correctly. Emptying is handled server-side by each namenode.

        LGTM +1 will commit shortly.

        Show
        andrew.wang Andrew Wang added a comment - Gotcha. Looking at the Shell code, it resolves the child filesystem before creating the Trash, and calls getTrashRoot correctly. Emptying is handled server-side by each namenode. LGTM +1 will commit shortly.
        Hide
        andrew.wang Andrew Wang added a comment -

        Committed to trunk.

        Manoj, do you want to do another JIRA to mark this method as @Deprecated in branch-2?

        Show
        andrew.wang Andrew Wang added a comment - Committed to trunk. Manoj, do you want to do another JIRA to mark this method as @Deprecated in branch-2?
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10616 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10616/)
        HADOOP-13721. Remove stale method ViewFileSystem#getTrashCanLocation. (wang: rev aee538be6c2ab324de4d7834cd3347959272de01)

        • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10616 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10616/ ) HADOOP-13721 . Remove stale method ViewFileSystem#getTrashCanLocation. (wang: rev aee538be6c2ab324de4d7834cd3347959272de01) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java

          People

          • Assignee:
            manojg Manoj Govindassamy
            Reporter:
            manojg Manoj Govindassamy
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development