Skip to content

HADOOP-16910 Adding file system counters in ABFS #1881

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Mar 23, 2020
Merged

HADOOP-16910 Adding file system counters in ABFS #1881

merged 9 commits into from
Mar 23, 2020

Conversation

@mehakmeet
Copy link
Contributor

  • Write_ops
  • Read_ops
  • Bytes_written (already updated)
  • Bytes_Read (already updated)

Change-Id: I77349fdd158babd66df665713201fa9c8606f191

NOTICE

Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute

@mehakmeet
Copy link
Contributor Author

mehakmeet commented Mar 6, 2020 •
edited
Loading

test run : mvn -T 1C -Dparallel-tests=abfs clean verify
Region : East US, West US

Failed tests(Container configurations based) :

[ERROR] Failures: [ERROR] ITestGetNameSpaceEnabled.testNonXNSAccount:59->Assert.assertFalse:64->Assert.assertTrue:41->Assert.fail:88 Expecting getIsNamespaceEnabled() return false
[ERROR] Errors: [ERROR] ITestGetNameSpaceEnabled.testFailedRequestWhenCredentialsNotCorrect:91->AbstractAbfsIntegrationTest.getFileSystem:197 ? InvalidConfigurationValue

steveloughran
steveloughran requested changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

Looks good, some minor nits, but production code fine. Made some suggestions to the tests

mukund-thakur
mukund-thakur requested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@mukund-thakur mukund-thakur left a comment

Choose a reason for hiding this comment

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

Some minor changes. Rest all looks good.

@mehakmeet mehakmeet requested a review from steveloughran March 11, 2020 12:16
mukund-thakur
mukund-thakur reviewed Mar 12, 2020
View reviewed changes
Copy link
Contributor

@mukund-thakur mukund-thakur left a comment

Choose a reason for hiding this comment

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

No sure why Yetus is not running. There might be some more checkstyle issues.

@mehakmeet mehakmeet closed this Mar 12, 2020
@mehakmeet mehakmeet reopened this Mar 12, 2020
mukund-thakur
mukund-thakur approved these changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@mukund-thakur mukund-thakur left a comment

Choose a reason for hiding this comment

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

LGTM. Please resolve the comments which you have fixed and keep others which you think still require some discussion.

steveloughran
steveloughran requested changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

looking good, apart from some minor style nits, the only thing needed is to skip the statistics updates if the class is null.
We should all be aware that stats are updated in the thread invoking the read/write call, and not the stats counter of the thread which opened it. This is not very important here...but be aware HBase does share the same stream instance across threads, as does impala. Stats updates will be spread across threads there -there is nothing we can do about it

...ools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ordering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought ordering was right?
java
non org.apache
org.apache
static

mukund-thakur
mukund-thakur requested changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@mukund-thakur mukund-thakur left a comment

Choose a reason for hiding this comment

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

New method is not generic enough. Introduce an "String Operation" parameter.

@mehakmeet mehakmeet requested a review from steveloughran March 18, 2020 07:41
@apache apache deleted a comment from hadoop-yetus Mar 18, 2020
steveloughran
steveloughran requested changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

all production code good, just final test tuning, all about making sure we close streams

steveloughran
steveloughran requested changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

all production code good, just final test tuning, all about making sure we close streams

@mehakmeet mehakmeet requested a review from steveloughran March 18, 2020 12:26
steveloughran
steveloughran requested changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@steveloughran steveloughran left a comment

Choose a reason for hiding this comment

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

all good -nearly there...just need to create and pass down a logger, so that things will get logged. I know, it doesn't seem important, but when you field support calls, you really appreciate the logging

mukund-thakur
mukund-thakur reviewed Mar 23, 2020
View reviewed changes
...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStreamStatistics.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this logging required ? @steveloughran

@mehakmeet mehakmeet mentioned this pull request Mar 23, 2020
Merged
mukund-thakur
mukund-thakur reviewed Mar 23, 2020
View reviewed changes
.../hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemOauth.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Use IOUtils.cleanupWithLogger() here as well. Or you can just say this code is old.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.close() does this already.

@mehakmeet mehakmeet force-pushed the FileSystem-Counters branch from a607839 to afba6cc Compare March 23, 2020 06:58
Mehakmeet Singh added 8 commits March 23, 2020 13:45
CDPD-8485 Adding file system counters
bfb4afa 
- Write_ops
- Read_ops
- Bytes_written (already updated)
- Bytes_Read (already updated)

Change-Id: I77349fdd158babd66df665713201fa9c8606f191
Fixing Review Comments
ff01bcf
Meaningful assert messages
3e59ca5
minor nits in test
ecc4c80
minor nits in test
536d1e1
Fixing review comments
0e33bea
closing streams
72f7181
Logging, closing, modifying
6b28cf2
@mehakmeet mehakmeet force-pushed the FileSystem-Counters branch from afba6cc to 6b28cf2 Compare March 23, 2020 08:16
@steveloughran steveloughran merged commit e2c7ac7 into apache:trunk Mar 23, 2020
@steveloughran
Copy link
Contributor

+1, merged to trunk. thanks!

@aajisaka
Copy link
Member

Hi @mehakmeet and @steveloughran
Failed to compile TestAbfsOutputStream.java after the merge. Would the fix it?

@mehakmeet
Copy link
Contributor Author

mehakmeet commented Mar 24, 2020 •
edited
Loading

@aajisaka
Yes, I'll be fixing it.
https://issues.apache.org/jira/browse/HADOOP-16934

@mehakmeet mehakmeet mentioned this pull request Mar 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mukund-thakur mukund-thakur mukund-thakur requested changes

@steveloughran steveloughran steveloughran requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mehakmeet @steveloughran @aajisaka @mukund-thakur