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-16379: S3AInputStream#unbuffer should merge input stream stats into fs-wide stats #983
Conversation
🎊 +1 overall
This message was automatically generated. |
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AUnbuffer.java
Outdated
Show resolved
Hide resolved
inputStream = fs.open(dest); | ||
|
||
// Sanity check to make sure the stream statistics are 0 | ||
assertEquals(0, |
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.
use org.apache.hadoop.fs.s3a.S3ATestUtils.MetricDiff for measuring and asserting on state of metrics, as it generates meaningful error messages on assert failures.
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AUnbuffer.java
Outdated
Show resolved
Hide resolved
public void setup() throws Exception { | ||
super.setup(); | ||
dest = path("ITestS3AUnbuffer"); | ||
describe("ITestS3AUnbuffer"); | ||
try (FSDataOutputStream outputStream = getFileSystem().create(dest, true)) { |
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.
ContractTestUtils.writeDataset
// instance of S3AInstrumentation | ||
Configuration conf = createConfiguration(); | ||
S3AFileSystem fs = new S3AFileSystem(); | ||
fs.initialize(getFileSystem().getUri(), conf); |
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.
needs to be deleted in a finally clause
Also, I don't think it is needed at all. MetricDiff is designed to do asserts over changes in statistics.
|
||
@Override | ||
public void teardown() throws Exception { | ||
getFileSystem().delete(dest, true); |
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.
superclass deleteTestDirInTeardown() should do this. If it doesn't, guard the call with checks for filesystem and destpath not being null, so it won't hide the stack traces of a failure in setup
LGTM, some changes around the testing. Checkstyle has a complaint which needs to be addressed |
… into fs-wide stats
6ef1aed
to
1928b33
Compare
🎊 +1 overall
This message was automatically generated. |
@steveloughran addressed your comments. Hadoop QA looks happy as well. Re-ran the S3A tests. |
LGTM: +1 |
committed to trunk. thanks! |
HADOOP-16379: S3AInputStream#unbuffer should merge input stream stats into fs-wide stats
InputStreamStatistics
calledmerge
which allows users to periodically merge the stats into the fs-wide statsunbuffer
merges the stream stats into the fs-wide statsTesting: