[
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:
-------------------------------------
Matt-
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 change.
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.
