-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-17016. Adding Common Counters in ABFS #1991
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
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
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 looks fine. Please fix the comments.
@steveloughran Could you also take a look once? Thanks.
🎊 +1 overall
This message was automatically generated. |
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
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.
Some minor changes pending. Rest looks fine.
🎊 +1 overall
This message was automatically generated. |
we're ok with imports of constants; its the package/method imports that are most brittle. Just use .* |
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.
LTGM
Key proposed changes
- just use .* for the constant imports
- move to interface + implementation of the metrics class
- proposed test cleanup by adding custom assertion and so eliminate duplicate code.
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
fs.open(createFilePath); | ||
fs.append(createFilePath); | ||
fs.rename(createFilePath, destCreateFilePath); | ||
assertTrue(fs.rename(createFilePath, destCreateFilePath)); |
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: There's a ContractTestUtils rename which is a bit more informative on rename failure, but I Don't see this failing enough to worry about it.
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.
I'll make sure to use that going forward.
Ok, all is good. I'd point you at ContractTestUtils for future rename/exists checks in tests, as it has better error reporting. +1 |
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42
merged to trunk |
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42 (cherry picked from commit 7f486f0)
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42 (cherry picked from commit 7f486f0)
Contributed by: Mehakmeet Singh. Change-Id: Ib84e7a42f28e064df4c6204fcce33e573360bf42 (cherry picked from commit 7f486f0)
Common Counters to be Added in ABFS.
test run: mvn -T 1C -Dparallel-tests=abfs clean verify
Region: East US, West US
UT:
IT: