HDFS-16917 Add transfer rate quantile metrics for DataNode reads #5397
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
rdingankar
force-pushed
the
HDFS-16917
branch
5 times, most recently
from
3c9d4b3 to
3f8ce0c
Compare
xinglin
reviewed
Feb 16, 2023
| * @return the number of megabytes/second of the transfer rate | ||
| */ | ||
| public static long transferRateMBs(long bytes, long durationMS) { | ||
| if (durationMS == 0) { |
There was a problem hiding this comment.
if it is <= 0, just return -1? Let's add a check for bytes as well.
There was a problem hiding this comment.
I dont feel we should handle other cases. This is a Utils method and any unexpected data should be left for the client to interpret. For some clients the negative values might even make sense.
The idea behind handling for durationMS = 0 is to take care of DivideByZero for cases when data transfer did not happen.
There was a problem hiding this comment.
can we specify our function as: "we expect both inputs to be positive. Otherwise, this function will return -1".
Then returning -1 is a clear signal we don't know how to handle such inputs.
xinglin
reviewed
Feb 16, 2023
| @@ -61,6 +61,8 @@ public class DataNodeMetrics { | |||
| @Metric MutableCounterLong bytesRead; | |||
| @Metric("Milliseconds spent reading") | |||
| MutableCounterLong totalReadTime; | |||
| @Metric MutableRate bytesReadTransferRate; | |||
There was a problem hiding this comment.
nit: rename to readTransferRateMBs?
xinglin
reviewed
Feb 16, 2023
| @@ -370,6 +370,7 @@ Each metrics record contains tags such as SessionId and Hostname as additional i | |||
| |:---- |:---- | | |||
| | `BytesWritten` | Total number of bytes written to DataNode | | |||
| | `BytesRead` | Total number of bytes read from DataNode | | |||
| | `BytesReadTransferRate`*num*`s(50/75/90/95/99)thPercentileRate` | The 50/75/90/95/99th percentile of the transfer rate of bytes read from the DataNode. The transfer rate is measured in megabytes per second. | | |||
There was a problem hiding this comment.
nit: rename to ReadTransferRateMBs?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rdingankar
force-pushed
the
HDFS-16917
branch
2 times, most recently
from
4271417 to
b3267f3
Compare
|
The UTs failing in the build are unrelated to my change. Verified locally that the same tests fail after rebasing to current trunk. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
xinglin
reviewed
Feb 20, 2023
| if (durationMS == 0) { | ||
| durationMS = 1; | ||
| } | ||
| return bytes / (1024 * 1024) * 1000 / durationMS; |
There was a problem hiding this comment.
We probably need to change the division to be done based on float/double. Then return a float/double as well.
There was a problem hiding this comment.
The Metrics take long, so eventually we will have to convert to long anyways.
There was a problem hiding this comment.
I would make this bytes per second and let the consumer do the conversion.
There was a problem hiding this comment.
If we want to have the consumer do the conversion, then we can just keep it to bytes/milliseconds. bytes and milliseconds are the units used for most of the metrics in HDFS.
Thoughts @xinglin @mkuchenbecker ?
| @@ -1122,6 +1124,7 @@ public void copyBlock(final ExtendedBlock block, | |||
| datanode.metrics.incrBytesRead((int) read); | |||
| datanode.metrics.incrBlocksRead(); | |||
| datanode.metrics.incrTotalReadTime(duration); | |||
| datanode.metrics.addReadTransferRateMBs(DFSUtil.transferRateMBs(read, duration)); | |||
There was a problem hiding this comment.
You will be publishing the metrics, including the -1 here. Assuming duration is 0, I would expect bytes read to be 0 as well (only instance with sub millisecond transfers should be when there's an error prior to the transfer).
Bytes Read 0, Duration 0: Return 0.
Bytes Read N, duration 0: Exception which we log.
Bytes Read N, duration K: Normal
There was a problem hiding this comment.
I was thinking we can publish -1 for the error scenarios which can be visualized from the graphs as well.
For duration of 0, changing it to 1 will give us the number of bytes if transferred.
There was a problem hiding this comment.
updated to log for cases when we get unexpected data.
- Bytes read is negative
- Duration is negative or 0.
Since we are logging these cases not emitting the data to metrics.
| * @param durationMS duration in milliseconds | ||
| * @return the number of megabytes/second of the transfer rate | ||
| */ | ||
| public static long transferRateMBs(long bytes, long durationMS) { |
There was a problem hiding this comment.
Added unit tests
rdingankar
force-pushed
the
HDFS-16917
branch
from
b3267f3 to
4ddbda7
Compare
rdingankar
force-pushed
the
HDFS-16917
branch
from
4ddbda7 to
b00ea94
Compare
rdingankar
force-pushed
the
HDFS-16917
branch
from
31407d7 to
27a79aa
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| @@ -632,6 +632,11 @@ public void readBlock(final ExtendedBlock block, | |||
| datanode.metrics.incrBytesRead((int) read); | |||
| datanode.metrics.incrBlocksRead(); | |||
| datanode.metrics.incrTotalReadTime(duration); | |||
| if (read < 0 || duration <= 0) { | |||
There was a problem hiding this comment.
You duplicated this logic, I would put behind a private helper so people don't forget this check.
There was a problem hiding this comment.
moved to a Utils method
xinglin
approved these changes
Feb 23, 2023
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @xinglin and @mkuchenbecker for the reviews and @omalley for helping to merge the change. |
|
@omalley Can you also help in backporting the PR to branch 3.3 |
omalley
pushed a commit
that referenced
this pull request
Feb 28, 2023
Co-authored-by: Ravindra Dingankar <rdingankar@linkedin.com>
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
…che#5397) Co-authored-by: Ravindra Dingankar <rdingankar@linkedin.com>
|
@rdingankar after updating our cluster to hadoop 3.3.6, we are now seeing |
tomasbanet
added a commit
to tomasbanet/hadoop
that referenced
this pull request
Nov 2, 2023
…ads (apache#5397)" Reason for revert: After updating cluster to hadoop 3.3.6, all datanodes started to log `WARN hdfs.DFSUtil: Unexpected value for data transfer bytes=XXXX duration=0`, every minute, where XXXX ranges from ~4-5kb to ~200kb. This reverts commit 0ca5686.
|
I agree this is way too verbose. I created https://issues.apache.org/jira/browse/HDFS-17262 |
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Transfer rate metric for datanode reads will be calculated as the rate at which bytes are read ( bytes per ms )
With quantiles we will get a distribution of this rate which will be helpful in identifying slow datanodes.
How was this patch tested?
Updated Unit Tests to verify metrics will be emitted.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?