Details

    • Type: New Feature New Feature
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Release Note:
      New fs -find command

      Description

      Both sysadmins and users make frequent use of the unix 'find' command, but Hadoop has no correlate. Without this, users are writing scripts which make heavy use of hadoop dfs -lsr, and implementing find one-offs. I think hdfs -lsr is somewhat taxing on the NameNode, and a really slow experience on the client side. Possibly an in-NameNode find operation would be only a bit more taxing on the NameNode, but significantly faster from the client's point of view?

      The minimum set of options I can think of which would make a Hadoop find command generally useful is (in priority order):

      • -type (file or directory, for now)
      • -atime/-ctime-mtime (... and -creationtime?) (both + and - arguments)
      • -print0 (for piping to xargs -0)
      • -depth
      • -owner/-group (and -nouser/-nogroup)
      • -name (allowing for shell pattern, or even regex?)
      • -perm
      • -size

      One possible special case, but could possibly be really cool if it ran from within the NameNode:

      • -delete
        The "hadoop dfs -lsr | hadoop dfs -rm" cycle is really, really slow.

      Lower priority, some people do use operators, mostly to execute -or searches such as:

      • find / (-nouser -or -nogroup)

      Finally, I thought I'd include a link to the Posix spec for find

      1. HADOOP-8989.patch
        140 kB
        Jonathan Allen
      2. HADOOP-8989.patch
        140 kB
        Jonathan Allen
      3. HADOOP-8989.patch
        140 kB
        Jonathan Allen
      4. HADOOP-8989.patch
        146 kB
        Jonathan Allen
      5. HADOOP-8989.patch
        141 kB
        Jonathan Allen
      6. HADOOP-8989.patch
        130 kB
        Jonathan Allen
      7. HADOOP-8989.patch
        131 kB
        Jonathan Allen
      8. HADOOP-8989.patch
        126 kB
        Jonathan Allen
      9. HADOOP-8989.patch
        126 kB
        Jonathan Allen
      10. HADOOP-8989.patch
        400 kB
        Jonathan Allen
      11. HADOOP-8989.patch
        400 kB
        Jonathan Allen
      12. HADOOP-8989.patch
        400 kB
        Jonathan Allen
      13. HADOOP-8989.patch
        400 kB
        Jonathan Allen
      14. HADOOP-8989.patch
        361 kB
        Jonathan Allen
      15. HADOOP-8989.patch
        338 kB
        Jonathan Allen
      16. HADOOP-8989.patch
        331 kB
        Jonathan Allen
      17. HADOOP-8989.patch
        297 kB
        Jonathan Allen
      18. HADOOP-8989.patch
        252 kB
        Jonathan Allen
      19. HADOOP-8989.patch
        230 kB
        Jonathan Allen
      20. HADOOP-8989.patch
        140 kB
        Jonathan Allen
      21. HADOOP-8989.patch
        98 kB
        Jonathan Allen

        Issue Links

          Activity

          Hide
          Raghu Angadi added a comment -

          > The "hadoop dfs -lsr | hadoop dfs -rm" cycle is really, really slow.

          This is a general problems for all commands that take multiple files as arguments. Namenode could support some 'container' versions for some commands(RPCs) like 'rm', 'chmod' so that it does not need to sync edits log for each file.

          Show
          Raghu Angadi added a comment - > The "hadoop dfs -lsr | hadoop dfs -rm" cycle is really, really slow. This is a general problems for all commands that take multiple files as arguments. Namenode could support some 'container' versions for some commands(RPCs) like 'rm', 'chmod' so that it does not need to sync edits log for each file.
          Hide
          Raghu Angadi added a comment -

          Regd 'dfs -rm', another option could be to give an option to use a thread pool at the client. This will be client-only fix and can easily increase the removal rate by 5-10 times (using 10-20 threads).. this code could be common for many commands.

          Show
          Raghu Angadi added a comment - Regd 'dfs -rm', another option could be to give an option to use a thread pool at the client. This will be client-only fix and can easily increase the removal rate by 5-10 times (using 10-20 threads).. this code could be common for many commands.
          Hide
          Koji Noguchi added a comment -

          this code could be common for many commands.

          It would be nice if this works with distcp.
          Copy any files created in the past 1 week, skip temporary files/dirs (HADOOP-3890), etc.

          Show
          Koji Noguchi added a comment - this code could be common for many commands. It would be nice if this works with distcp. Copy any files created in the past 1 week, skip temporary files/dirs ( HADOOP-3890 ), etc.
          Hide
          dhruba borthakur added a comment -

          Another very convenient way to achieve this is to fuse-mount hdfs on the gateway machines. http://wiki.apache.org/hadoop/MountableHDFS

          The HDFS file system appears as a local file system, e.g. /export/hdfs. An user's script that works on other linux file systems will then work on HDFS too.

          Show
          dhruba borthakur added a comment - Another very convenient way to achieve this is to fuse-mount hdfs on the gateway machines. http://wiki.apache.org/hadoop/MountableHDFS The HDFS file system appears as a local file system, e.g. /export/hdfs. An user's script that works on other linux file systems will then work on HDFS too.
          Hide
          Allen Wittenauer added a comment -

          It would be good to get some traction on this old issue. It is becoming more and more prevalent that ops teams need to be able to do operations on the whole of the file system (such as quota reporting, and find). While tools such as the offline image viewer are nice, some tasks really do require relatively real time updates. There are also the issues about handing out the image file from a security and practicality perspective.

          Show
          Allen Wittenauer added a comment - It would be good to get some traction on this old issue. It is becoming more and more prevalent that ops teams need to be able to do operations on the whole of the file system (such as quota reporting, and find). While tools such as the offline image viewer are nice, some tasks really do require relatively real time updates. There are also the issues about handing out the image file from a security and practicality perspective.
          Hide
          Philip Zeyliger added a comment -

          If you're willing to deal with XML, http://namenode/listPaths?recursive=yes will give you an XML file with all the files on that namenode. You could probably convert your find query into XQuery.

          Show
          Philip Zeyliger added a comment - If you're willing to deal with XML, http://namenode/listPaths?recursive=yes will give you an XML file with all the files on that namenode. You could probably convert your find query into XQuery.
          Hide
          Richard Theige added a comment -

          XML output is not Service Engineer/Sys Admin friendly.

          We need to see traction on this find feature.
          While Grid Service Engineers are not Java dev's, what is wrong with doing something like this snippet:

          ======

          public boolean show(FileSystem fs, Path p) throws Exception

          { FileStatus stat = fs.getFileStatus(p); System.out.printf("%s%s %d %12s %8s %14d %14d %s\n", (stat.isDir() ? "d" : "-"), stat.getPermission().toString(), stat.getReplication(), stat.getOwner(), stat.getGroup(), stat.getLen(), stat.getModificationTime(), stat.getPath().toUri().getPath() ); // Recurse on a directory if ( stat.isDir() ) stat(p); return(true); }

          }

          Show
          Richard Theige added a comment - XML output is not Service Engineer/Sys Admin friendly. We need to see traction on this find feature. While Grid Service Engineers are not Java dev's, what is wrong with doing something like this snippet: ====== public boolean show(FileSystem fs, Path p) throws Exception { FileStatus stat = fs.getFileStatus(p); System.out.printf("%s%s %d %12s %8s %14d %14d %s\n", (stat.isDir() ? "d" : "-"), stat.getPermission().toString(), stat.getReplication(), stat.getOwner(), stat.getGroup(), stat.getLen(), stat.getModificationTime(), stat.getPath().toUri().getPath() ); // Recurse on a directory if ( stat.isDir() ) stat(p); return(true); } }
          Hide
          dhruba borthakur added a comment -

          There are usually a set of unix shell commands that one would like to use on HDFS. If they are mostly for system adminsitrators, can you mount hdfs using fuse and then run the shell command? Doing this allows running any unixy command on hdfs without running specialized HDFS code.

          Show
          dhruba borthakur added a comment - There are usually a set of unix shell commands that one would like to use on HDFS. If they are mostly for system adminsitrators, can you mount hdfs using fuse and then run the shell command? Doing this allows running any unixy command on hdfs without running specialized HDFS code.
          Hide
          Pierre-Alexandre Meyer added a comment -

          I have implemented a find command as a standalone utility. See http://github.com/pierre/hfind

          -delete is not implemented yet, for safety purposes. As soon as I get more testing, it should be trivial to add.

          More details at http://pub.mouraf.org/blog/2010/09/hfind-a-find-utility-for-hadoop/

          Show
          Pierre-Alexandre Meyer added a comment - I have implemented a find command as a standalone utility. See http://github.com/pierre/hfind -delete is not implemented yet, for safety purposes. As soon as I get more testing, it should be trivial to add. More details at http://pub.mouraf.org/blog/2010/09/hfind-a-find-utility-for-hadoop/
          Hide
          Harsh J added a comment -

          Lately I also came across @fsfiii's https://github.com/fsfiii/hadoop-find

          Show
          Harsh J added a comment - Lately I also came across @fsfiii's https://github.com/fsfiii/hadoop-find
          Hide
          Daryn Sharp added a comment -

          I see a few problems with relying on an adjunct ruby script for finds. For one, it requires ruby being installed on systems. Second, it requires hadoop to certify which versions of ruby we support and/or have tested. Maybe these aren't big issue but it's something to keep in mind.

          I'm more worried about needing it to be kept in sync with hadoop releases. That could be solved by adopting the script into the core, but it's still another moving piece that needs/should be kept in sync with FsShell semantics and output.

          The redesigned shell in 23 will make adding a find a rather easy task. I've long been meaning to add find but haven't had the cycles. In fact, yesterday I was really wishing I had find.

          Show
          Daryn Sharp added a comment - I see a few problems with relying on an adjunct ruby script for finds. For one, it requires ruby being installed on systems. Second, it requires hadoop to certify which versions of ruby we support and/or have tested. Maybe these aren't big issue but it's something to keep in mind. I'm more worried about needing it to be kept in sync with hadoop releases. That could be solved by adopting the script into the core, but it's still another moving piece that needs/should be kept in sync with FsShell semantics and output. The redesigned shell in 23 will make adding a find a rather easy task. I've long been meaning to add find but haven't had the cycles. In fact, yesterday I was really wishing I had find.
          Hide
          Jonathan Allen added a comment -

          First draft of an fs -find command. Still a work in progress but what's there should work so feel free to play with it and let me have any comments.

          Show
          Jonathan Allen added a comment - First draft of an fs -find command. Still a work in progress but what's there should work so feel free to play with it and let me have any comments.
          Hide
          Jonathan Allen added a comment -

          Not complete but what's there is stable and could be reviewed if somebody wants to look at it. I'll just be adding expressions and tidying up the documentation now.

          The following expressions are implemented as per the posix definition: -a, -atime, -depth, -group, -mtime, -name, -newer, -nogroup, -not, -nouser, -o, -perm, -print, -prune, -size, -type, -user.

          I haven't included the following posix expressions as they don't look applicable here: -xdev, -links, -ctime.

          I've still got to add -exec and any other non-posix extensions that look useful.

          Show
          Jonathan Allen added a comment - Not complete but what's there is stable and could be reviewed if somebody wants to look at it. I'll just be adding expressions and tidying up the documentation now. The following expressions are implemented as per the posix definition: -a, -atime, -depth, -group, -mtime, -name, -newer, -nogroup, -not, -nouser, -o, -perm, -print, -prune, -size, -type, -user. I haven't included the following posix expressions as they don't look applicable here: -xdev, -links, -ctime. I've still got to add -exec and any other non-posix extensions that look useful.
          Hide
          Daryn Sharp added a comment -

          Wow, nice complement of expressions. I'll try to review soon.

          Show
          Daryn Sharp added a comment - Wow, nice complement of expressions. I'll try to review soon.
          Hide
          Jonathan Allen added a comment -

          Patch is now pretty much code complete - I just need to tidy some things up and do the documentation. It now handles symlinks (with -L and -H options) and includes the following expressions (in addition to those listed earlier): -amin (-atime in minutes), -blocksize, -exec, -ok, -mmin (-mtime in minutes), -replicas, -iname (case insensitive -name), -class (allows runtime pluggable expression classes).

          Show
          Jonathan Allen added a comment - Patch is now pretty much code complete - I just need to tidy some things up and do the documentation. It now handles symlinks (with -L and -H options) and includes the following expressions (in addition to those listed earlier): -amin (-atime in minutes), -blocksize, -exec, -ok, -mmin (-mtime in minutes), -replicas, -iname (case insensitive -name), -class (allows runtime pluggable expression classes).
          Hide
          wolfgang hoschek added a comment -

          I gave it a try and this patch is awesome. Would be really useful to add the following options as defined here: http://linux.die.net/man/1/find

          -regextype "java" (no need for other regex flavours here, but should be possible to add classic flavours later)
          -regex
          -maxdepth
          -mindepth
          -printf

          Show
          wolfgang hoschek added a comment - I gave it a try and this patch is awesome. Would be really useful to add the following options as defined here: http://linux.die.net/man/1/find -regextype "java" (no need for other regex flavours here, but should be possible to add classic flavours later) -regex -maxdepth -mindepth -printf
          Hide
          Allen Wittenauer added a comment -

          Has anyone studied the impact on the NN yet?

          Show
          Allen Wittenauer added a comment - Has anyone studied the impact on the NN yet?
          Hide
          wolfgang hoschek added a comment -

          In addition, would be good to have a -starttime <dateTime> <dateTimePattern> option to express the time related options (e.g. -mmin, -mtime, -amin, -atime) to work relative to a specific absolute timestamp (say, give me all files modified since yesterday midnight) instead of relative to whatever "now" happens to be at the time of command execution. The dateTimePattern might be ISO 8601 by default, and could be set to any java.text.SimpleDateFormat format pattern.

          The FindOptions class already seems to foresee such usage. It's just a matter of exposing it at the command level.

          Show
          wolfgang hoschek added a comment - In addition, would be good to have a -starttime <dateTime> <dateTimePattern> option to express the time related options (e.g. -mmin, -mtime, -amin, -atime) to work relative to a specific absolute timestamp (say, give me all files modified since yesterday midnight) instead of relative to whatever "now" happens to be at the time of command execution. The dateTimePattern might be ISO 8601 by default, and could be set to any java.text.SimpleDateFormat format pattern. The FindOptions class already seems to foresee such usage. It's just a matter of exposing it at the command level.
          Hide
          Daryn Sharp added a comment -

          Sorry, this fell off my radar. I've been swamped.

          Has anyone studied the impact on the NN yet?

          W/o having reviewed the patch, I initially don't think this could be any worse than ls -R.

          Show
          Daryn Sharp added a comment - Sorry, this fell off my radar. I've been swamped. Has anyone studied the impact on the NN yet? W/o having reviewed the patch, I initially don't think this could be any worse than ls -R.
          Hide
          Jonathan Allen added a comment -

          Allen - I haven't done any specific studies of the NN impact but I would expect it to be the same as any of the existing recursive command (e.g. ls -R). The code uses the existing recursion code and applies the expressions to the returned FileStatus on the client side. The -exec option obviously impacts the NN but no more than if you ran the command directly (though you don't have the JVM start-up overhead so the rate of NN messages will be higher).

          Wolfgang - I'll look at adding those option expressions in (I got to the point where it looked like I'd be adding expressions forever so I decided to stop and see what people asked for).

          Daryn - if you do get a chance to review this then the main code is pretty much finished (though I need to sort out the formatting before submitting properly) but I've reworked the unit tests since uploading the last patch (I'll upload a new at the weekend). I still need to update the documentation web page and add some stuff in the CLI test suites.

          Show
          Jonathan Allen added a comment - Allen - I haven't done any specific studies of the NN impact but I would expect it to be the same as any of the existing recursive command (e.g. ls -R). The code uses the existing recursion code and applies the expressions to the returned FileStatus on the client side. The -exec option obviously impacts the NN but no more than if you ran the command directly (though you don't have the JVM start-up overhead so the rate of NN messages will be higher). Wolfgang - I'll look at adding those option expressions in (I got to the point where it looked like I'd be adding expressions forever so I decided to stop and see what people asked for). Daryn - if you do get a chance to review this then the main code is pretty much finished (though I need to sort out the formatting before submitting properly) but I've reworked the unit tests since uploading the last patch (I'll upload a new at the weekend). I still need to update the documentation web page and add some stuff in the CLI test suites.
          Hide
          Jonathan Allen added a comment -

          unit tests tidied up, code style tidied up CLI and HDFS CLI test added

          Show
          Jonathan Allen added a comment - unit tests tidied up, code style tidied up CLI and HDFS CLI test added
          Hide
          Parameswaran Raman added a comment -

          Is this feature available yet? If not, any timelines?
          I ran into a use-case today where it would be easier to have 'find' support in hadoop dfs. "hadoop fs -ls -R runs out of memory!"

          Show
          Parameswaran Raman added a comment - Is this feature available yet? If not, any timelines? I ran into a use-case today where it would be easier to have 'find' support in hadoop dfs. "hadoop fs -ls -R runs out of memory!"
          Hide
          Jonathan Allen added a comment -

          ready for review

          Show
          Jonathan Allen added a comment - ready for 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/12561115/HADOOP-8989.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 40 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.cli.TestHDFSCLI

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1880//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1880//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/12561115/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 40 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.cli.TestHDFSCLI +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1880//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1880//console This message is automatically generated.
          Hide
          Jonathan Allen added a comment -

          test failure fixed

          Show
          Jonathan Allen added a comment - test failure fixed
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 40 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1885//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1885//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/12561136/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 40 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1885//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1885//console This message is automatically generated.
          Hide
          Luke Lu added a comment -

          Thanks for the great looking patch, Jonathan! The code is clean and test is comprehensive. One question though, in BaseExpression#getFileStatus for the follow symlink case, don't you think that you should use resolvePath instead just getSymlink?

          Show
          Luke Lu added a comment - Thanks for the great looking patch, Jonathan! The code is clean and test is comprehensive. One question though, in BaseExpression#getFileStatus for the follow symlink case, don't you think that you should use resolvePath instead just getSymlink?
          Hide
          Jonathan Allen added a comment -

          Luke, I assume you mean something like ' Path linkedFile = item.fs.resolvePath(fileStatus.getSymlink());' (or am I missing something?). Patch updated to include this and add some tests for the BaseExpression class.

          Show
          Jonathan Allen added a comment - Luke, I assume you mean something like ' Path linkedFile = item.fs.resolvePath(fileStatus.getSymlink());' (or am I missing something?). Patch updated to include this and add some tests for the BaseExpression class.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 41 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1889//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1889//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/12561197/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 41 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1889//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1889//console This message is automatically generated.
          Hide
          Caleb Jones added a comment -

          I've submitted a patch for HADOOP-9195 (DateTimePathFilter based on mtime) which may have some functionality similar to this.

          Show
          Caleb Jones added a comment - I've submitted a patch for HADOOP-9195 (DateTimePathFilter based on mtime) which may have some functionality similar to this.
          Hide
          Jonathan Allen added a comment -

          Luke - did you get a chance to finish reviewing this?

          Show
          Jonathan Allen added a comment - Luke - did you get a chance to finish reviewing this?
          Hide
          Luke Lu added a comment -

          Sorry Jonathan, I was hoping Daryn could take a look as he did the fs shell refactor

          Anyway, a few nits after a quick pass (more later):

          1. get rid of the @inheritDoc comment for all the inner classes, which are implementations.
          2. avoid using concrete classes in method signatures e.g. use List<String> args instead of LinkedList<String> args. I think this is largely due to the refactor done in HADOOP-7202. I think we should fix this in the base classes as well. Daryn, what do you think?

          I don't think HADOOP-9195 (though independently useful) would help the implementation here.

          Show
          Luke Lu added a comment - Sorry Jonathan, I was hoping Daryn could take a look as he did the fs shell refactor Anyway, a few nits after a quick pass (more later): get rid of the @inheritDoc comment for all the inner classes, which are implementations. avoid using concrete classes in method signatures e.g. use List<String> args instead of LinkedList<String> args. I think this is largely due to the refactor done in HADOOP-7202 . I think we should fix this in the base classes as well. Daryn, what do you think? I don't think HADOOP-9195 (though independently useful) would help the implementation here.
          Hide
          Daryn Sharp added a comment -

          For some reason I thought this was committed so it fell off my radar. I will try to squeeze out some time for reviewing.

          Show
          Daryn Sharp added a comment - For some reason I thought this was committed so it fell off my radar. I will try to squeeze out some time for reviewing.
          Hide
          Jonathan Allen added a comment -

          Luke - a couple of questions about your comments:
          1) where are you thinking of here? I can't find any @inheritDoc on an inner class so I feel I'm missing something? I notice inheritDoc isn't used very much throughout the code, is it generally frowned on?
          2) completely agree with the thought but this isn't something I can fix without changing nearly all of the fs.shell classes and this jira seems the wrong place to do that; happy to be guided by you and Daryn if you think I should.

          Show
          Jonathan Allen added a comment - Luke - a couple of questions about your comments: 1) where are you thinking of here? I can't find any @inheritDoc on an inner class so I feel I'm missing something? I notice inheritDoc isn't used very much throughout the code, is it generally frowned on? 2) completely agree with the thought but this isn't something I can fix without changing nearly all of the fs.shell classes and this jira seems the wrong place to do that; happy to be guided by you and Daryn if you think I should.
          Hide
          Daryn Sharp added a comment -

          I don't remember, but I think LinkedList was used in signatures because of methods unique to that class. I'd recommend any large sweeping changes be considered on another jira.

          Show
          Daryn Sharp added a comment - I don't remember, but I think LinkedList was used in signatures because of methods unique to that class. I'd recommend any large sweeping changes be considered on another jira.
          Hide
          Jonathan Allen added a comment -

          I've removed all of the @inheritDoc references (misunderstanding on part about when they were needed, didn't realise javadoc automatically inherited).

          Updated the web site documentation to use the format.

          Having a more detailed look at the use of LinkedList in the Command method signatures. It seems to be used as a mixture of List and Deque in different subclasses and so it's not a straightforward replacement with an interface.

          Show
          Jonathan Allen added a comment - I've removed all of the @inheritDoc references (misunderstanding on part about when they were needed, didn't realise javadoc automatically inherited). Updated the web site documentation to use the format. Having a more detailed look at the use of LinkedList in the Command method signatures. It seems to be used as a mixture of List and Deque in different subclasses and so it's not a straightforward replacement with an interface.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 41 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2198//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2198//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/12569735/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 41 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2198//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2198//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Initial comments:

          • Why is setCommandFactory exposed, and why is FsShell setting it in the instance?
          • Use of static blocks is discouraged because it leads to bizarre problems during class loading. Is there an easy way to avoid them?
          • "recognised" should be "recognized"
          • You can retrieve options = cf.getOpts instead of individually querying each option and building a new set.
          • Why is parsePathData checking for (arg == null)? That shouldn't happen since isEmpty is already being checked?
          • processPaths really shouldn't be overridden. It was only exposed for some difficult to handle ls behavior. The logic should be split into recursePath and processPath.
          • I'd suggest against making the expression classes public.

          I still need to study the expression handling.

          Show
          Daryn Sharp added a comment - Initial comments: Why is setCommandFactory exposed, and why is FsShell setting it in the instance? Use of static blocks is discouraged because it leads to bizarre problems during class loading. Is there an easy way to avoid them? "recognised" should be "recognized" You can retrieve options = cf.getOpts instead of individually querying each option and building a new set. Why is parsePathData checking for (arg == null) ? That shouldn't happen since isEmpty is already being checked? processPaths really shouldn't be overridden. It was only exposed for some difficult to handle ls behavior. The logic should be split into recursePath and processPath . I'd suggest against making the expression classes public. I still need to study the expression handling.
          Hide
          Jonathan Allen added a comment -

          Some quick answers to your questions ...

          The CommandFactory is added into the Command so that it can be used by the -exec expression (it needs to know what class to pass the found files into). It seemed cleaner to do it this way than to call FsShell.run for each file. In hindsight I think the factory should be setting it rather than FsShell (in the same place it calls setConf).

          Should be able to avoid most of the static blocks. As an aside, why are static blocks worse than initialising static variables? (or are static variables other than constants also bad?)

          (arg == null) needs to be checked because there may be a null on the Deque, nothing to do with args being empty or not. I'm not sure how a null would have got on the Deque but the method shouldn't be making assumptions about that.

          I needed to override processPaths so that I could implement the -depth option. processPaths always processes the current item before recursing the directory. It looks like somebody has added a postProcessPath method since I started this so that might solve the problem, I'll revisit.

          I should get a chance to action the comments later this week.

          Show
          Jonathan Allen added a comment - Some quick answers to your questions ... The CommandFactory is added into the Command so that it can be used by the -exec expression (it needs to know what class to pass the found files into). It seemed cleaner to do it this way than to call FsShell.run for each file. In hindsight I think the factory should be setting it rather than FsShell (in the same place it calls setConf). Should be able to avoid most of the static blocks. As an aside, why are static blocks worse than initialising static variables? (or are static variables other than constants also bad?) (arg == null) needs to be checked because there may be a null on the Deque, nothing to do with args being empty or not. I'm not sure how a null would have got on the Deque but the method shouldn't be making assumptions about that. I needed to override processPaths so that I could implement the -depth option. processPaths always processes the current item before recursing the directory. It looks like somebody has added a postProcessPath method since I started this so that might solve the problem, I'll revisit. I should get a chance to action the comments later this week.
          Hide
          Jonathan Allen added a comment -

          Patch updated following initial review.

          Use of static blocks minimised (though unfortunately still necessary in some places).

          Command.processPaths wasn't able to recurse through symbolic links; I've updated it to include a isPathRecursable extension method (basic implementation is just isDirectory but Find extends it to check symlinks).

          Rewriting the use of processPaths showed up some problems with the implementation of symbolic link following. This has now been fixed.

          Other review comments covered in previous comment.

          Show
          Jonathan Allen added a comment - Patch updated following initial review. Use of static blocks minimised (though unfortunately still necessary in some places). Command.processPaths wasn't able to recurse through symbolic links; I've updated it to include a isPathRecursable extension method (basic implementation is just isDirectory but Find extends it to check symlinks). Rewriting the use of processPaths showed up some problems with the implementation of symbolic link following. This has now been fixed. Other review comments covered in previous comment.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 42 new or modified test files.

          +1 tests included appear to have a timeout.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.datanode.TestDataDirs

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2304//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2304//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/12572993/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 42 new or modified test files. +1 tests included appear to have a timeout. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.datanode.TestDataDirs +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2304//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2304//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 42 new or modified test files.

          +1 tests included appear to have a timeout.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2330//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2330//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/12573783/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 42 new or modified test files. +1 tests included appear to have a timeout. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2330//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2330//console This message is automatically generated.
          Hide
          Mark Miller added a comment -

          Lot's of watchers, Hadoop QA was happy, anyone willing to tip this in?

          Show
          Mark Miller added a comment - Lot's of watchers, Hadoop QA was happy, anyone willing to tip this in?
          Hide
          Hadoop QA added a comment -

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3447//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/12573783/HADOOP-8989.patch against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3447//console This message is automatically generated.
          Hide
          Jonathan Allen added a comment -

          It was waiting for a review, I know Daryn was part way through but needed to put it aside for a while and I don't know if he had a chance to get back to it. I'll sort out the problems it seems to have against the latest trunk and submit a new patch (hopefully won't require major changes).

          Show
          Jonathan Allen added a comment - It was waiting for a review, I know Daryn was part way through but needed to put it aside for a while and I don't know if he had a chance to get back to it. I'll sort out the problems it seems to have against the latest trunk and submit a new patch (hopefully won't require major changes).
          Hide
          Jonathan Allen added a comment -

          Patch updated to build with latest trunk (only minor change from version patch).

          Show
          Jonathan Allen added a comment - Patch updated to build with latest trunk (only minor change from version patch).
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 42 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3450//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3450//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/12624208/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 42 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3450//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3450//console This message is automatically generated.
          Hide
          Akira AJISAKA added a comment -

          The patch mostly looks good. I built with the patch and verified some expressions worked.
          This feature is also very useful to search files/dirs in a fsimage with new OfflineImageViewer, and therefore I want this patch will be merged in.
          Minor nit: There's some trailing whitespaces.

          Show
          Akira AJISAKA added a comment - The patch mostly looks good. I built with the patch and verified some expressions worked. This feature is also very useful to search files/dirs in a fsimage with new OfflineImageViewer, and therefore I want this patch will be merged in. Minor nit: There's some trailing whitespaces.
          Hide
          Jonathan Allen added a comment -

          Trailing whitespace removed from patch

          Show
          Jonathan Allen added a comment - Trailing whitespace removed from patch
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 42 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3756//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3756//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/12639169/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 42 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3756//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3756//console This message is automatically generated.
          Hide
          Uma Maheswara Rao G added a comment -

          On quick look at your patch:
          I think you get the results as full paths

          hdfs://127.0.0.1:59894/texttest
          hdfs://127.0.0.1:59894/texttest/file..bz2
          hdfs://127.0.0.1:59894/texttest/file.deflate
          hdfs://127.0.0.1:59894/texttest/file.gz
          

          This should be relative paths I think from where you are finding, no?

          Overall your work looks to be great. I think having bulk patch may be had to review and many things may be overlooked. How about splitting this to small patches?

          /**
           * Implements the -mindepth expression for the
           * {@link org.apache.hadoop.fs.shell.find.Find} command.
           */
          final class MaxDepth extends BaseExpression {
          

          javadoc should say maxdepth

          Show
          Uma Maheswara Rao G added a comment - On quick look at your patch: I think you get the results as full paths hdfs://127.0.0.1:59894/texttest hdfs://127.0.0.1:59894/texttest/file..bz2 hdfs://127.0.0.1:59894/texttest/file.deflate hdfs://127.0.0.1:59894/texttest/file.gz This should be relative paths I think from where you are finding, no? Overall your work looks to be great. I think having bulk patch may be had to review and many things may be overlooked. How about splitting this to small patches? /** * Implements the -mindepth expression for the * {@link org.apache.hadoop.fs.shell.find.Find} command. */ final class MaxDepth extends BaseExpression { javadoc should say maxdepth
          Hide
          Akira AJISAKA added a comment -

          Thanks Jonathan Allen for updating the patch.

          How about splitting this to small patches?

          +1 (non-binding) for splitting. If you don't have a time to do this, I'm willing to help.

          /**
           * Construct a Print {@Expression} with an operational ASCII NULL
           * suffix.
           */
            private Print(boolean appendNull) {
          

          @Expression should be @link Expression.

              public int apply(int current, int shift, int value) {
                return current | (value << shift);
              };
          

          In Perm.java some semicolons can be removed.

          Show
          Akira AJISAKA added a comment - Thanks Jonathan Allen for updating the patch. How about splitting this to small patches? +1 (non-binding) for splitting. If you don't have a time to do this, I'm willing to help. /** * Construct a Print {@Expression} with an operational ASCII NULL * suffix. */ private Print( boolean appendNull) { @Expression should be @link Expression . public int apply( int current, int shift, int value) { return current | (value << shift); }; In Perm.java some semicolons can be removed.
          Hide
          Jonathan Allen added a comment -

          I'm happy to split into small patches but could do with advice on how to do this. Are you suggesting creating extra issues or multiple files attached to this one or something else?

          Show
          Jonathan Allen added a comment - I'm happy to split into small patches but could do with advice on how to do this. Are you suggesting creating extra issues or multiple files attached to this one or something else?
          Hide
          Akira AJISAKA added a comment -

          I suggest to create extra issues. You can create sub-tasks and attach a small patch on each issue.

          Show
          Akira AJISAKA added a comment - I suggest to create extra issues. You can create sub-tasks and attach a small patch on each issue.
          Hide
          Jonathan Allen added a comment -

          OK, I should have time to do that over the weekend. How do I make sure the patches get applied in the correct order? (ie patch 2 depends on patch 1, etc).

          Show
          Jonathan Allen added a comment - OK, I should have time to do that over the weekend. How do I make sure the patches get applied in the correct order? (ie patch 2 depends on patch 1, etc).
          Hide
          Akira AJISAKA added a comment -

          I think you can add a "depends upon" link to an issue to clarify the order.
          ex.) If the patch 2 at issue 2 depends on the patch 1 at issue 1, it's better to add "depends upon issue 1" link to issue 2.

          Show
          Akira AJISAKA added a comment - I think you can add a "depends upon" link to an issue to clarify the order. ex.) If the patch 2 at issue 2 depends on the patch 1 at issue 1, it's better to add "depends upon issue 1" link to issue 2.
          Hide
          Jonathan Allen added a comment -

          Patch striped back to minimum - command plus name, print and and operations. I'll add the rest back in as separate patches.

          Show
          Jonathan Allen added a comment - Patch striped back to minimum - command plus name, print and and operations. I'll add the rest back in as separate patches.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 9 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3860//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3860//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/12642147/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3860//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3860//console This message is automatically generated.
          Hide
          Akira AJISAKA added a comment -

          Thanks Jonathan Allen for splitting the patch!

          I think you get the results as full paths

          hdfs://127.0.0.1:59894/texttest
          hdfs://127.0.0.1:59894/texttest/file..bz2
          hdfs://127.0.0.1:59894/texttest/file.deflate
          hdfs://127.0.0.1:59894/texttest/file.gz
          

          This should be relative paths I think from where you are finding, no?

          Would you please reflect the above Uma Maheswara Rao G's comment?

          A new comment:

          • The tests for -iname option is needed.

          Two minor nits:

          • Typo in TestAnd.java
            // test both expressions failining
            
          • There're some lines over 80 characters in TestFind.java
          Show
          Akira AJISAKA added a comment - Thanks Jonathan Allen for splitting the patch! I think you get the results as full paths hdfs: //127.0.0.1:59894/texttest hdfs: //127.0.0.1:59894/texttest/file..bz2 hdfs: //127.0.0.1:59894/texttest/file.deflate hdfs: //127.0.0.1:59894/texttest/file.gz This should be relative paths I think from where you are finding, no? Would you please reflect the above Uma Maheswara Rao G 's comment? A new comment: The tests for -iname option is needed. Two minor nits: Typo in TestAnd.java // test both expressions failining There're some lines over 80 characters in TestFind.java
          Hide
          Jonathan Allen added a comment -

          Actioned review comments.

          Show
          Jonathan Allen added a comment - Actioned review comments.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 10 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3928//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3928//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/12643862/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 10 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3928//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3928//console This message is automatically generated.
          Hide
          Jonathan Allen added a comment -

          Akira AJISAKA are you able to continue the review of this patch?

          Show
          Jonathan Allen added a comment - Akira AJISAKA are you able to continue the review of this patch?
          Hide
          Akira AJISAKA added a comment -

          Jonathan Allen, sorry for late response. I'll review the patch by tomorrow.

          Show
          Akira AJISAKA added a comment - Jonathan Allen , sorry for late response. I'll review the patch by tomorrow.
          Hide
          Akira AJISAKA added a comment -

          Thanks for updating the patch. I compiled with the patch and tried some options.

          1. When I executed -print0 option, newlines are included in the output as follows:

          # hdfs dfs -find /user/root -name '*.txt' -print0
          abc.txt
          def.txt
          

          Newlines shouldn't be included in -print0 option.

          2. In Find.java, spaces should be used instead of tabs.

              // Initialize the static variables.
              EXPRESSIONS = new Class[] {
              		// Operator Expressions
              		And.class,
              		// Action Expressions
              		Print.class,
              		// Navigation Expressions
              		// Matcher Expressions
              		Name.class
              		};
          

          Minor nits:

          3. In Expressions.java,

            /**
             * Returns the precendence of this expression
             * (only applicable to operators).
             */
          

          precendence should be precedence.

          4. There are some trailing white spaces in Find.java.

          5. In Print.java,

            private Print(boolean appendNull) {
              super();
              setUsage(USAGE);
              setHelp(HELP);
              setAppendNull(appendNull);
            }
          
            private void setAppendNull(boolean appendNull) {
              this.appendNull = appendNull;
            }
          

          can be simplified as

            private Print(boolean appendNull) {
              super();
              setUsage(USAGE);
              setHelp(HELP);
              this.appendNull = appendNull;
            }
          

          6.

            /**
             * Construct a Print {@link Expression} with an operational ASCII NUL
             * suffix.
             */
          

          operational should be optional?

          Show
          Akira AJISAKA added a comment - Thanks for updating the patch. I compiled with the patch and tried some options. 1. When I executed -print0 option, newlines are included in the output as follows: # hdfs dfs -find /user/root -name '*.txt' -print0 abc.txt def.txt Newlines shouldn't be included in -print0 option. 2. In Find.java, spaces should be used instead of tabs. // Initialize the static variables. EXPRESSIONS = new Class [] { // Operator Expressions And.class, // Action Expressions Print.class, // Navigation Expressions // Matcher Expressions Name.class }; Minor nits: 3. In Expressions.java, /** * Returns the precendence of this expression * (only applicable to operators). */ precendence should be precedence. 4. There are some trailing white spaces in Find.java. 5. In Print.java, private Print( boolean appendNull) { super (); setUsage(USAGE); setHelp(HELP); setAppendNull(appendNull); } private void setAppendNull( boolean appendNull) { this .appendNull = appendNull; } can be simplified as private Print( boolean appendNull) { super (); setUsage(USAGE); setHelp(HELP); this .appendNull = appendNull; } 6. /** * Construct a Print {@link Expression} with an operational ASCII NUL * suffix. */ operational should be optional?
          Hide
          Jonathan Allen added a comment -

          Thanks for the review comments, all now actioned.

          After realising the -print0 should use '\0' in place of '\n' rather than as well as I've changed the constructor to just take the appropriate suffix.

          Show
          Jonathan Allen added a comment - Thanks for the review comments, all now actioned. After realising the -print0 should use '\0' in place of '\n' rather than as well as I've changed the constructor to just take the appropriate suffix.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 10 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4054//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4054//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/12650144/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 10 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4054//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4054//console This message is automatically generated.
          Hide
          Akira AJISAKA added a comment -

          Thanks for updating the patch! Mostly looks good. I verified -print0 option and the following command worked well.

          # hdfs dfs -find /user/root/ -name "file*" -print0 | xargs -0 hdfs dfs -rm
          

          Minor comment:

            private static String[] HELP =
                { "Finds all files that match the specified expression and "
                    + "applies selected actions to them." };
          

          Would you change the code like other commands? i.e.

            private static String[] HELP =
                { "Finds all files that match the specified expression and",
                  "applies selected actions to them." };
          

          In addition, can you please include a document in the patch for keeping consistency between source code and document? (i.e. Would you split HADOOP-10580?)

          Uma Maheswara Rao G, do you have any comments on this patch? I'll appreciate your reviews.

          Show
          Akira AJISAKA added a comment - Thanks for updating the patch! Mostly looks good. I verified -print0 option and the following command worked well. # hdfs dfs -find /user/root/ -name "file*" -print0 | xargs -0 hdfs dfs -rm Minor comment: private static String [] HELP = { "Finds all files that match the specified expression and " + "applies selected actions to them." }; Would you change the code like other commands? i.e. private static String [] HELP = { "Finds all files that match the specified expression and" , "applies selected actions to them." }; In addition, can you please include a document in the patch for keeping consistency between source code and document? (i.e. Would you split HADOOP-10580 ?) Uma Maheswara Rao G , do you have any comments on this patch? I'll appreciate your reviews.
          Hide
          Daryn Sharp added a comment -

          Do you actually need the changes to Command and CommandFactory to add a new command?

          I'm not sure the path handling/expansion is consistent with the rest of the commands. Find is a bit odd in that paths come before all options, but I think there has to be a cleaner way to implement the parsing.

          Show
          Daryn Sharp added a comment - Do you actually need the changes to Command and CommandFactory to add a new command? I'm not sure the path handling/expansion is consistent with the rest of the commands. Find is a bit odd in that paths come before all options, but I think there has to be a cleaner way to implement the parsing.
          Hide
          Jonathan Allen added a comment -

          Daryn Sharp

          Do you actually need the changes to Command and CommandFactory to add a new command?

          Yes, I think I do.

          The change to Command#processPaths introduces a hook to allow recursion through something other than directories, specifically this allows Find to recurse through symbolic links when required. This could be implemented by overriding the whole processPaths method in Find but this seems likely to introduce maintenance problems going forwards (and in a previous comment you suggested that this shouldn't be done).

          The CommandFactory is added into the Command so that it can be used by the -exec expression (it needs to know what class to pass the found files into). It seemed cleaner to do it this way than to call FsShell.run for each file.

          They both seem fairly simple and safe changes that could be useful to other commands in the future.

          I'm not sure the path handling/expansion is consistent with the rest of the commands. Find is a bit odd in that paths come before all options, but I think there has to be a cleaner way to implement the parsing.

          Possibly, but I couldn't think of one. If you can see something I've missed then feel free to suggest it.

          Show
          Jonathan Allen added a comment - Daryn Sharp Do you actually need the changes to Command and CommandFactory to add a new command? Yes, I think I do. The change to Command#processPaths introduces a hook to allow recursion through something other than directories, specifically this allows Find to recurse through symbolic links when required. This could be implemented by overriding the whole processPaths method in Find but this seems likely to introduce maintenance problems going forwards (and in a previous comment you suggested that this shouldn't be done). The CommandFactory is added into the Command so that it can be used by the -exec expression (it needs to know what class to pass the found files into). It seemed cleaner to do it this way than to call FsShell.run for each file. They both seem fairly simple and safe changes that could be useful to other commands in the future. I'm not sure the path handling/expansion is consistent with the rest of the commands. Find is a bit odd in that paths come before all options, but I think there has to be a cleaner way to implement the parsing. Possibly, but I couldn't think of one. If you can see something I've missed then feel free to suggest it.
          Hide
          Jonathan Allen added a comment -

          Documentation and CLI tests added and review comment actioned.

          Show
          Jonathan Allen added a comment - Documentation and CLI tests added and review comment actioned.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.cli.TestCLI
          org.apache.hadoop.cli.TestHDFSCLI

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4099//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4099//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/12651267/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.cli.TestCLI org.apache.hadoop.cli.TestHDFSCLI +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4099//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4099//console This message is automatically generated.
          Hide
          Jonathan Allen added a comment -

          Fixed CLI tests.

          Show
          Jonathan Allen added a comment - Fixed CLI tests.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.cli.TestCLI
          org.apache.hadoop.hdfs.server.namenode.TestCacheDirectives

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4136//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4136//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/12651882/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.cli.TestCLI org.apache.hadoop.hdfs.server.namenode.TestCacheDirectives +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4136//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4136//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4137//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4137//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/12651889/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4137//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4137//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          I think you can simplify the processing flow by having processOptions also parse out the expressions, leaving only the paths in the list. The method is intended to strip all option related arguments.

          If no paths are given, shouldn't it default to the pwd instead of aborting?

          Show
          Daryn Sharp added a comment - I think you can simplify the processing flow by having processOptions also parse out the expressions, leaving only the paths in the list. The method is intended to strip all option related arguments. If no paths are given, shouldn't it default to the pwd instead of aborting?
          Hide
          Jonathan Allen added a comment -

          I think you can simplify the processing flow by having processOptions also parse out the expressions, leaving only the paths in the list.

          Yes, I think you're right about that. I hadn't thought about skipping past the path arguments and stripping the remaining expressions out of the list (though obvious now it's been suggested).

          If no paths are given, shouldn't it default to the pwd instead of aborting?

          That doesn't seem to be the standard find behaviour (or at least not on my Mac)

          $ find -print
          find: illegal option -- p
          usage: find [-H | -L | -P] [-EXdsx] [-f path] path ... [expression]
                 find [-H | -L | -P] [-EXdsx] -f path [path ...] [expression]
          
          Show
          Jonathan Allen added a comment - I think you can simplify the processing flow by having processOptions also parse out the expressions, leaving only the paths in the list. Yes, I think you're right about that. I hadn't thought about skipping past the path arguments and stripping the remaining expressions out of the list (though obvious now it's been suggested). If no paths are given, shouldn't it default to the pwd instead of aborting? That doesn't seem to be the standard find behaviour (or at least not on my Mac) $ find -print find: illegal option -- p usage: find [-H | -L | -P] [-EXdsx] [-f path] path ... [expression] find [-H | -L | -P] [-EXdsx] -f path [path ...] [expression]
          Hide
          Jonathan Allen added a comment -

          If no paths are given, shouldn't it default to the pwd instead of aborting?

          I've now checked the Linux implementation of find and this does default to pwd so I'll make the change to this (it's only a single line either way). The Posix find specifies path as mandatory but I think we tend to favour Linux - correct me if I'm wrong about this.

          Show
          Jonathan Allen added a comment - If no paths are given, shouldn't it default to the pwd instead of aborting? I've now checked the Linux implementation of find and this does default to pwd so I'll make the change to this (it's only a single line either way). The Posix find specifies path as mandatory but I think we tend to favour Linux - correct me if I'm wrong about this.
          Hide
          Allen Wittenauer added a comment -

          We should work on trying to avoid GNU or Linux -isms, but in this particular case it seems reasonable to default to pwd.

          Show
          Allen Wittenauer added a comment - We should work on trying to avoid GNU or Linux -isms, but in this particular case it seems reasonable to default to pwd.
          Hide
          Jonathan Allen added a comment -

          Moved argument processing to processOptions and defaulted path to pwd.

          Show
          Jonathan Allen added a comment - Moved argument processing to processOptions and defaulted path to pwd.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestCrcCorruption

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4166//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4166//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/12652301/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestCrcCorruption +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4166//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4166//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 12 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4188//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4188//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/12653073/HADOOP-8989.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4188//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4188//console This message is automatically generated.

            People

            • Assignee:
              Jonathan Allen
              Reporter:
              Marco Nicosia
            • Votes:
              3 Vote for this issue
              Watchers:
              41 Start watching this issue

              Dates

              • Created:
                Updated:

                Development