-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19254: Implement bulk delete command as hadoop fs command operation #7197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
steveloughran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests in {{AbstractContractBulkDeleteTest}} to verify it actually works
- create temp file/files < page size, assert deleted
- pass in file list > page size, with an existing file at the end. Assert exception raised and last file in list retained
- do same with file list
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/FsCommand.java
Outdated
Show resolved
Hide resolved
mukund-thakur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some comments
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
mukund-thakur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better. I think more tests are required as discussed before.
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| Path baseDir = new Path(testRootDir, deletionDir); | ||
| List<String> listOfPaths = new ArrayList<>(); | ||
| for(int i = 0; i < 100; i++) { | ||
| Path p = new Path(baseDir, baseFileName + i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are utilities for create files and directories in ContractTestUtils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are empty files, should I use touch from ContractTestUtils here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you can use touch() from there. Actually there is createFiles(count) but that is in S3ATestUtils.
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
|
you indentation causing lot of checkstyle failures. |
This comment was marked as outdated.
This comment was marked as outdated.
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
production side nearly done, leaving only test tuning
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
mukund-thakur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some exception related tests pending. Otherwise mostly looking good.
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good, just agreeing with mukund that logging the path on failure is probably a good idea
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestBulkDeleteCommand.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
mukund-thakur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1 pending that clarification. Please resolve open comments before merging.
...common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/BulkDeleteCommand.java
Show resolved
Hide resolved
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
Description of PR
Implement bulk delete command as hadoop fs command operation
How was this patch tested?
Patch was tested in local by building the entire stack and against a s3 bucket.
Added a unit test for functionality testing.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?