HDFS-17641. Add badly distributed blocks metric #7123
Merged
Conversation
This file contains 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. |
Comment on lines
229
to
230
| // insert a block with too many replicas to make badly distributed | ||
| assertAdded(queues, block_badly_distributed, 2, 0, 1); |
There was a problem hiding this comment.
Why does a block having too many replicas make it badly distributed? Wouldn't that just make the block overreplicated which we have existing metrics for?
There was a problem hiding this comment.
That is true, however I think it might be the way to test it as in the BlockQueue level it is the only type which has more replicas than required.
From discussion, thoughts are that the impl is assuming nothing calls getPriority to begin with unless it needs replication, and if it has sufficient replicas then that must be because it's badly distributed.
|
💔 -1 overall
This message was automatically generated. |
aQuigs
reviewed
Nov 5, 2024
Comment on lines
+4741
to
+4746
|
|
||
| @Metric({"BadlyDistBlocks", "Number of Badly Distributed Blocks"}) | ||
| public long getBadlyDistributedBlocksCount() { | ||
| // not locking | ||
| return blockManager.getBadlyDistributedBlocksCount(); | ||
| } |
There was a problem hiding this comment.
This metric looks like a duplicate of below. Is that intentional?
There was a problem hiding this comment.
I think both are needed since the BlockManager gets the count, BadlyDistBlocks exposes metric for Metrics2 and theres also BadlyDistributedBlocks metric for MBean
There was a problem hiding this comment.
Also getBadlyDistributedBlocksCount is not locking while getBadlyDistributedBlocks isnt
Comment on lines
+5194
to
+5198
| public long getBadlyDistributedBlocksCount() { | ||
| // not locking | ||
| return this.neededReconstruction.getBadlyDistributedBlocks(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks redundant with getBadlyDistributedBlocks()?
| * Return total number of erasure coded block groups. | ||
| * @return total number of erasure coded block groups. |
There was a problem hiding this comment.
Nit - this is the only change in this file and all the methods above have the old syntax too. Maybe we should scope this for a different PR to keep the diff clean (and address the entire class)
There was a problem hiding this comment.
If I don't include this javadoc fails. However even when I do include this a different javadoc fails for essentially same issue. Should I chase down all the issues or leave it be?
There was a problem hiding this comment.
@PrateekSane We should remove this from the patch as it is unrelated to your change. Sorry, I missed this earlier. Everything else looks good and will commit after this change. Thank you!
...ject/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ECBlockGroupStats.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ctrezzo
reviewed
Nov 12, 2024
There was a problem hiding this comment.
@PrateekSane please see the build failures in the latest yetus run. The patch failed to apply (https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7123/4/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs-rbf.txt) and had some checkstyle issues. Please address such that we get a clean run.
Looks like the issue for the build is:
src/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/NamenodeBeanMetrics.java:[387,3] method does not override or implement a method from a supertype
Thank you!
|
💔 -1 overall
This message was automatically generated. |
ctrezzo
reviewed
Nov 25, 2024
There was a problem hiding this comment.
@PrateekSane the latest push seems to have introduced whitespace changes unrelated to the change. Can you revert those as well. Thank you!
Nevermind! I see that there are changes in the files now.
ctrezzo
approved these changes
Nov 25, 2024
|
💔 -1 overall
This message was automatically generated. |
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
Add metric BadlyDistrbutedBlocks to the list of metrics which tracks the number of badly distributed blocks in the queue.
How was this patch tested?
Add Badly distributed blocks to TestLowRedundancy blocks to ensure properly tracked.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?