-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-17471. ABFS to collect IOStatistics #2731
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
|
Need to remove DurationTrackers from streams, since now I am collecting them from AbfsRestOperation. |
|
Looks great! |
|
Extended in the execute() method of Requests seem to return 404 with FileSystem not found exception. Tried to debug didn't get much info with that as well. Have you faced this issue before with DurationTrackingFactory @steveloughran ? |
|
My mistake. I was translating IOException into RuntimeException when it was expected to throw IOException while setup. |
steveloughran
left a comment
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'm submitting a review but I wrote it 10 days ago and forgot to hit "submit review"; possibly outdated now. sorry
...op-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsCounters.java
Outdated
Show resolved
Hide resolved
...ools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java
Outdated
Show resolved
Hide resolved
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java
Outdated
Show resolved
Hide resolved
...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsDurationTrackers.java
Outdated
Show resolved
Hide resolved
...hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsInputStreamStatistics.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.
again, can we use assertThatStatisticMean()?
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 am not able to add .isGreatherThan(<SOME_DOUBLE>) after assertThatStatisticMean(), I guess because this is an ObjectAssert<MeanStatistic> rather than DoubleAssert?
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.
yes, I guess so. We'd probably need some conversion to get the mean/sum/count value. Let's not worry about that here
...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsNetworkStatistics.java
Outdated
Show resolved
Hide resolved
...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsNetworkStatistics.java
Outdated
Show resolved
Hide resolved
… remove conflicts
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
left a comment
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.
LGTM; one change -to throwing an UncheckedIOException when wrapping an IOE.
+1 pending that change
| } catch (AzureBlobFileSystemException aze) { | ||
| throw aze; | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Error while tracking Duration of an " |
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.
change to UncheckedIOException; there's stuff in our functional package which can unwrap that down to an IOE as we always know the inner fault is a non-null IOE
|
+1, merging. Thanks! |
|
🎊 +1 overall
This message was automatically generated. |
The ABFS Filesystem and its input and output streams now implement the IOStatisticSource interface and provide IOStatistics on their interactions with Azure Storage. This includes the min/max/mean durations of all REST API calls. Contributed by Mehakmeet Singh <mehakmeet.singh@cloudera.com>
The ABFS Filesystem and its input and output streams now implement the IOStatisticSource interface and provide IOStatistics on their interactions with Azure Storage. This includes the min/max/mean durations of all REST API calls. Contributed by Mehakmeet Singh <mehakmeet.singh@cloudera.com>
The ABFS Filesystem and its input and output streams now implement the IOStatisticSource interface and provide IOStatistics on their interactions with Azure Storage. This includes the min/max/mean durations of all REST API calls. Contributed by Mehakmeet Singh <mehakmeet.singh@cloudera.com>
The ABFS Filesystem and its input and output streams now implement the IOStatisticSource interface and provide IOStatistics on their interactions with Azure Storage. This includes the min/max/mean durations of all REST API calls. Note: This commit contains some changes from "CDPD-20510. HADOOP-17414. Magic committer files don't have the count of bytes written collected by spark" in DurationTrackerFactory interface to keep AbfsCountersImpl class from breaking. Contributed by Mehakmeet Singh Conflicts: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java Change-Id: I07df5f558b00a7e548eda33dd6503f14337d5fdc
Tested by: mvn -T 16 -Dparallel-tests=abfs clean verify
Region: East US
Illegal argument due to not using oauth config for these tests.