[jira] [Commented] (HADOOP-8849) FileUtil#fullyDelete should grant the target directories +rwx permissions before trying to delete them

["Robert Joseph Evans (JIRA)" <ji...@apache.org>]

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

Robert Joseph Evans commented on HADOOP-8849:
---------------------------------------------

I am not sure that we actually want to do this change.  FileUtil.fullyDelete is called by RawLocalFileSystem.delete for recursive deletes.  With this change RawLocalFilesSystem will ignore permissions on recursive deletes if the user has the permission to change those permissions.  In all other places that I have seen the API used I think it is OK, but this is also a publicly visible API so I don't know who else this may cause problems with.  I would rather see a new API created separate from the original one, and the javadocs updated to explain the difference between the two APIs.  Perhaps something like

{code}
public static boolean fullyDelete(final File dir) {
  return fullyDelete(dir, false);
}

  /**
   * Delete a directory and all its contents.  If
   * we return false, the directory may be partially-deleted.
   * (1) If dir is symlink to a file, the symlink is deleted. The file pointed
   *     to by the symlink is not deleted.
   * (2) If dir is symlink to a directory, symlink is deleted. The directory
   *     pointed to by symlink is not deleted.
   * (3) If dir is a normal file, it is deleted.
   * (4) If dir is a normal directory, then dir and all its contents recursively
   *     are deleted.
   * @param dir the file or directory to be deleted
   * @param tryUpdatePerms true if permissions should be modified to delete a file.
   * @return true on success false on failure.
   */
public static boolean fullyDelete(final File dir, boolean tryUpdatePerms) {
 ...
{code}
                
> FileUtil#fullyDelete should grant the target directories +rwx permissions before trying to delete them
> ------------------------------------------------------------------------------------------------------
>
>                 Key: HADOOP-8849
>                 URL: https://issues.apache.org/jira/browse/HADOOP-8849
>             Project: Hadoop Common
>          Issue Type: Improvement
>            Reporter: Ivan A. Veselovsky
>            Assignee: Ivan A. Veselovsky
>            Priority: Minor
>         Attachments: HADOOP-8849-vs-trunk-4.patch
>
>
> 2 improvements are suggested for implementation of methods org.apache.hadoop.fs.FileUtil.fullyDelete(File) and org.apache.hadoop.fs.FileUtil.fullyDeleteContents(File):
>  
> 1) We should grant +rwx permissions the target directories before trying to delete them.
> The mentioned methods fail to delete directories that don't have read or execute permissions.
> Actual problem appears if an hdfs-related test is timed out (with a short timeout like tens of seconds), and the forked test process is killed, some directories are left on disk that are not readable and/or executable. This prevents next tests from being executed properly because these directories cannot be deleted with FileUtil#fullyDelete(), so many subsequent tests fail. So, its recommended to grant the read, write, and execute permissions the directories whose content is to be deleted.
> 2) Generic reliability improvement: we shouldn't rely upon File#delete() return value, use File#exists() instead. 
> FileUtil#fullyDelete() uses return value of method java.io.File#delete(), but this is not reliable because File#delete() returns true only if the file was deleted as a result of the #delete() method invocation. E.g. in the following code
> if (f.exists()) { // 1
>   return f.delete(); // 2
> }
> if the file f was deleted by another thread or process between calls "1" and "2", this fragment will return "false", while the file f does not exist upon the method return.
> So, better to write
> if (f.exists()) {
>   f.delete();
>   return !f.exists();
> }

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira