-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-18189 S3APrefetchingInputStream to support status probes when closed #5036
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
Tested against
|
🎊 +1 overall
This message was automatically generated. |
@steveloughran, requesting your review |
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 code all good, minor tweaks to the test cases needed
+ " should still support some status probes"); | ||
|
||
byte[] data = ContractTestUtils.dataset(SMALL_FILE_SIZE, 'a', 26); | ||
Path smallFile = path("testStatusProbesAfterClosingStream"); |
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.
you can just use path()
and one is built up from the method name. this is better as it avoids cut and paste bugs and if a build is parameterized, the parameterized string is used in the path (though tests fail if that string isn't a valid path any more)
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 my bad, should have used methodPath()
. You meant methodPath()
only, correct?
assertEquals("Stream stats retrieved through stream before and after closing should match", | ||
inputStreamStatistics, newInputStreamStatistics); | ||
|
||
assertFalse("Not supported with prefetch", in.seekToNewSource(10)); |
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.
nit: can you have the error message include "seekToNewSource()".
🎊 +1 overall
This message was automatically generated. |
Addressed both of above comments |
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.
+1, merging. thanks
…closed (apache#5036) Contributed by Viraj Jasani
…closed (#5036) Contributed by Viraj Jasani
…closed (#5036) Contributed by Viraj Jasani
No description provided.