Skip to content
This repository was archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-2037] Make local file system deleteDirectory api recursive#1474

Merged
motus merged 2 commits intoapache:masterfrom
dwaijam:dwaijam/delDirLocal
Jun 20, 2018
Merged

[REEF-2037] Make local file system deleteDirectory api recursive#1474
motus merged 2 commits intoapache:masterfrom
dwaijam:dwaijam/delDirLocal

Conversation

@dwaijam
Copy link
Contributor

@dwaijam dwaijam commented Jun 11, 2018

[REEF-2037] Introduce configurable azure blob exponential retry policy

DeleteDirectory of LocalFileSystem does not do recursive delete unlike other filesystem implementations

JIRA:
REEF-2037

Pull request:
This closes 1474

@dwaijam dwaijam changed the title Make local file system deleteDirectory api recursive [REEF-2037] Make local file system deleteDirectory api recursive Jun 11, 2018
Assert.False(File.Exists(childDirFileUri.AbsolutePath));
Assert.False(File.Exists(fileUri.AbsolutePath));
Assert.False(File.Exists(childDirUri.AbsolutePath));
Assert.False(File.Exists(directoryUri.AbsolutePath));
Copy link
Contributor

@tyclintw tyclintw Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asserting on the directory alone should suffice. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


In reply to: 194553352 [](ancestors = 194553352)

MakeRemoteTestFile(fs, fileUri);
var childDirFileUri = new Uri(directoryUri, $"{childDirName}/{fileName}");
MakeRemoteTestFile(fs, childDirFileUri);
fs.DeleteDirectory(directoryUri);
Copy link
Contributor

@tyclintw tyclintw Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block of code is very dense. Please reorg to make more readable.

i.e.

//Create directory

//Create sub file

//Create sub directory #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


In reply to: 194553594 [](ancestors = 194553594)

Copy link
Contributor

@tyclintw tyclintw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

Copy link
Contributor

@tyclintw tyclintw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@singlis
Copy link
Contributor

singlis commented Jun 16, 2018

@markusweimer, @motus, @tcondie -These changes look good to me, can one of you take a look?

Copy link
Contributor

@motus motus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Will test and merge now

Copy link
Contributor

@motus motus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've just noticed, though, that on Hadoop .DeleteDirectory() actually performs hdfs dfs -rmdir with no options -- that is, it will fail to delete non-empty directory. We probably want to make the directory removal behavior consistent across all file systems. Should we open a JIRA for it?

@dwaijam
Copy link
Contributor Author

dwaijam commented Jun 20, 2018

I created this - https://issues.apache.org/jira/browse/REEF-2038

@motus motus merged commit d20e391 into apache:master Jun 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants