-
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-18403. Fix FileSystem leak in ITestS3AAWSCredentialsProvider #4737
Conversation
Test results look good, tested against endpoint |
🎊 +1 overall
This message was automatically generated. |
@mukund-thakur could you please review this PR? |
@steveloughran could you please review as per convenience? nothing urgent. |
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 good.
assertNotNull(stat); | ||
assertEquals(testFile, stat.getPath()); | ||
try (FileSystem fs = FileSystem.newInstance(testFile.toUri(), conf)) { | ||
assertNotNull(fs); |
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.
Now as you are fixing this already, why don't we add error messages in the assert statements for better error reporting. thx.
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.
Done, thanks @mukund-thakur
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.
Sorry for adding extra work.
assertEquals(testFile, stat.getPath()); | ||
try (FileSystem fs = FileSystem.newInstance(testFile.toUri(), conf)) { | ||
assertNotNull("S3AFileSystem instance must not be null", fs); | ||
assertTrue(fs instanceof S3AFileSystem); |
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.
here as well please.
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.
Ah, missed it. Done, thanks
assertTrue(fs instanceof S3AFileSystem); | ||
FileStatus stat = fs.getFileStatus(testFile); | ||
assertNotNull("FileStatus with qualified path must not be null", stat); | ||
assertEquals(testFile, stat.getPath()); |
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.
and 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.
LGTM +1,
let's wait for yetus to finish.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…4737) Contributed By: Viraj Jasani
merged to branch-3.3 as well. |
…pache#4737) Contributed By: Viraj Jasani
No description provided.