-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-13294. Test hadoop fs shell against s3a #4006
base: trunk
Are you sure you want to change the base?
Conversation
🎊 +1 overall
This message was automatically generated. |
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, thanks for the assertions on file system as well as stdout
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.
Ready to approve with fixed checkstyle
/** | ||
* Test of hadoop fs shell against S3A | ||
*/ |
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.
This adds a new checkstyle violation, can you push a fix?
./hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFsShell.java:45:/**: First sentence should end with a period. [JavadocStyle]
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.
Sure, I will fix it.
@Test | ||
public void testFsShellFileOperations() throws IOException { |
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.
This method is too long, and raises a checkstyle violation.
./hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFsShell.java:141: @Test:3: Method length is 155 lines (max allowed is 150). [MethodLength]
Can we break it down? Maybe move the stream concatenation (cat, head, tail) to another method?
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.
Good suggestion, I will break them to another method.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
baedcb0
to
a8668b9
Compare
💔 -1 overall
This message was automatically generated. |
a8668b9
to
9ca8e5f
Compare
💔 -1 overall
This message was automatically generated. |
9ca8e5f
to
cc2cb89
Compare
🎊 +1 overall
This message was automatically generated. |
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 - one blocker on the root delete support.
I think this was already discussed in the HADOOP-13294...
I will update my PR for the new option and make a change to s3a files only.
So we will make it so only S3A opts-out of supporting root delete, and all other filesystems assume it is enabled by default? Sounds good!
cc2cb89
to
ede9377
Compare
💔 -1 overall
This message was automatically generated. |
…te root directory
… to delete root directory" This reverts commit 91a4e23313d5d9f8bcc7559cb337856cbd8f0ad6.
This reverts commit 9ca8e5fcdb9d8f4dda0281d4908e9e165ef96ca8.
ede9377
to
df6285a
Compare
💔 -1 overall
This message was automatically generated. |
Description of PR
There's no tests of hadoop -fs commands against S3a. With some tests we can see problems when using these commands with S3a (HADOOP-13294).
From the command list in FileSystemShell, I added tests for each of them, verify expected output from stdout, stderr and return code. Some tests might be a bit long because I'm trying to group the commands and run them together so it won't take too long.
Changes
How was this patch tested?
Tested in
eu-west-1
withmvn -Dparallel-tests -DtestsThreadCount=16 clean verify
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?