hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jakob Homan (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-6345) Refactor Trash::moveToTrash() and its accompanying poor tests
Date Wed, 09 Dec 2009 05:03:18 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-6345?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12787918#action_12787918

Jakob Homan commented on HADOOP-6345:

  Took a look at the patch.  Thanks for the contribution!

It looks like there are a couple of behavior changes introduced in the patch:
* Only return false if the item-to-be-deleted is already in the trash.  This seems like a
reasonable behavior.
* If unable to to create the directory in the trash, throw an exception rather than return
false.  This is also good, although I'm not sure we need the extra permission information
in the exception message.

I have two general concerns:
* It seems like the essential, convoluted logic of the moveToTrash method remains. I had thought
that this would be a good chance to clean up some of the nested try-catch blocks.  And the
existing for loop seems particularly confusing.
* Of slight concern to me is the change that prevents the user from deleting a file that's
in the trash already from the trash.  I guess now that we have -skipTrash there is a method
for a use to selectively purge items from the trash, but this will still be an incompatible

Other items:
* In moveToTrash, it may be better to include the FileNotFoundException explicitly in the
checked argument exceptions (before IOException).
* What is goal of making the conf, shell and trash variables fields? 
* Line 253: A file not being found does not necessarily imply an invalid path.  It may be
better to just phrase it as file not found.
* Line 269: It's a good idea to keep the log entry as well as the new exception; these entries
have proven valuable during debugging.
* Line 201: The test testMoveToTrashRenameSuccess appears to be testing a successful operation.
Should rename be included in the method name?

Minor formatting issues:
* There are several places where indenting is flush in different levels or code or not consistent
with our coding style.
* There are a few lines that go way beyond 80 chars, our usual limit
* It looks like a few tabs have made it into the patch file as well.  We just use spaces.
* Line 242: It's not necessary to log a warning if the trash is turned off; this is normal
behavior.  A log.debug line may be reasonable though.

In HDFS the TestTrash class is subclassed and run against an HDFS instance.  We'll need to
test these changes against that project as well.  This is a bit difficult right now with the
project split.  We need a good way of testing with a specific jar rather than having to check
in (even locally) a jar.

> Refactor Trash::moveToTrash() and its accompanying poor tests
> -------------------------------------------------------------
>                 Key: HADOOP-6345
>                 URL: https://issues.apache.org/jira/browse/HADOOP-6345
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: fs
>    Affects Versions: 0.20.1
>            Reporter: Jakob Homan
>            Assignee: Matt Ahrens
>         Attachments: HADOOP-6345.2.patch, HADOOP-6345.patch
> We've had several issues relating to the Trash and its access via the shell, none of
which were picked up by the unit tests.  The current moveToTrash method has 8 different ways
it can terminate and sometimes uses a false value and sometimes uses an exception to indicate
failure.  This method should be refactored to improve readability and testability, and new
tests written to exercise all possible code paths.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message