Skip to content
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

HDDS-2777. Add bytes read statistics to Ozone FileSystem implementation #382

Merged
merged 3 commits into from Dec 21, 2019

Conversation

fapifta
Copy link
Contributor

@fapifta fapifta commented Dec 20, 2019

NOTE: please forgive me and be aware the fact that I have created the branch with the wrong JIRA ID in the name, and the commit also contains a wrong JIRA ID, please fix it when commiting or if needed let me know and I recreate the PR...

What changes were proposed in this pull request?

The main aim is to account the number of bytes read FileSystem statistics.
Besides that the PR adds tests around the number of bytes read and number of bytes write, the number of read operations, and the number of write operations statistics.
Also there is a fix for the BasicOzoneFileSystem#open call which was accounted as a write operation before the patch.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-2777

How was this patch tested?

See added JUnit tests.

@fapifta fapifta changed the title HDDS-2277. Add bytes read statistics to Ozone FileSystem implementation HDDS-2777. Add bytes read statistics to Ozone FileSystem implementation Dec 20, 2019
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fapifta for this improvement. Really great test coverage.

Note that OzoneFSInputStream implements ByteBufferReadable, so there is a read(ByteBuffer) method, which is still not collecting stats. It can be fixed simply by changing it to call read instead of inputStream.read:

https://github.com/apache/hadoop-ozone/blob/b834fa48afef4ee4c73577c7af564e1e97cb9d5b/hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java#L96

@fapifta
Copy link
Contributor Author

fapifta commented Dec 20, 2019

Hi Attila,

thank you for the review, somehow I did not went down in the file enough and haven't noticed that method... However I tried to review all the implementations, I missed this one.

I have pushed the discussed modifications to the PR. And also added two new tests to cover the method as well.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fapifta for updating the patch. Confirmed unit test passes locally (since it's excluded by CI).

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM.
Nice test coverage for the Stats code.

@bharatviswa504 bharatviswa504 merged commit b5008d0 into apache:master Dec 21, 2019
@bharatviswa504
Copy link
Contributor

Thank You @fapifta for the contribution and @adoroszlai for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants