HDFS-17037. Consider nonDfsUsed when running balancer. #5715
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. |
Hexiaoqiao
reviewed
Jun 5, 2023
| @@ -104,21 +104,21 @@ void accumulateSpaces(DatanodeStorageReport r) { | |||
| for(StorageReport s : r.getStorageReports()) { | |||
| final StorageType t = s.getStorage().getStorageType(); | |||
| totalCapacities.add(t, s.getCapacity()); | |||
| totalUsedSpaces.add(t, s.getDfsUsed()); | |||
| totalUsedSpaces.add(t, (s.getCapacity() - s.getRemaining())); | |||
There was a problem hiding this comment.
It could not be true when using s.getCapacity() - s.getRemaining() directly. For instance, it will be negative if one storage with total capacity 10MB，but configured Capacity 1MB for hdfs, and it remain 8MB after run for a while, then s.getCapacity() - s.getRemaining() will be -710241024.
There was a problem hiding this comment.
Thanks for your suggestion. I checked that FsVolumeImpl#configuredCapacity is set only by FsVolumeImpl#setCapacityForTesting, and FsVolumeImpl#setCapacityForTesting is used for unit tests. So s.getCapacity() - s.getRemaining() will never be negative.
There was a problem hiding this comment.
Got it. Suggest to leave some annotations about this here.
| } | ||
| } | ||
| return capacity == 0L? null: dfsUsed*100.0/capacity; | ||
| return capacity == 0L? null: totalUsed*100.0/capacity; |
There was a problem hiding this comment.
should fix the codestyle.
| @@ -104,21 +104,21 @@ void accumulateSpaces(DatanodeStorageReport r) { | |||
| for(StorageReport s : r.getStorageReports()) { | |||
There was a problem hiding this comment.
IMO, the BalancingPolicy.Pool also have the same issue.
There was a problem hiding this comment.
The goal of BalancingPolicy.Pool is to balance each pool in each datanode. If we use (capacity - remaining)/capacity to compute util here, it must be greater or equal than blockPoolUsed/capacity. In this way, each block pool will get the same util, so the initial goal will not be achieved. All in all, s.getRemaining() is at Node level, so I think it not very properly to modify BalancingPolicy.Pool.
There was a problem hiding this comment.
Use blockPoolUsed / (remaining + blockPoolUsed) may be a proper solution. What about your opinion?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jun 7, 2023
| for(StorageReport s : r.getStorageReports()) { | ||
| if (s.getStorage().getStorageType() == t) { | ||
| capacity += s.getCapacity(); | ||
| dfsUsed += s.getDfsUsed(); | ||
| totalUsed += (s.getCapacity() - s.getRemaining()); |
There was a problem hiding this comment.
From #5708 report, s.getRemaining() could be negative in some corner case. So if we should defend it here?
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jun 8, 2023
| @@ -104,21 +104,21 @@ void accumulateSpaces(DatanodeStorageReport r) { | |||
| for(StorageReport s : r.getStorageReports()) { | |||
| final StorageType t = s.getStorage().getStorageType(); | |||
| totalCapacities.add(t, s.getCapacity()); | |||
| totalUsedSpaces.add(t, s.getDfsUsed()); | |||
| totalUsedSpaces.add(t, (s.getCapacity() - s.getRemaining())); | |||
There was a problem hiding this comment.
It was not necessary to involve extra outer brackets here.
totalUsedSpaces.add(t, (s.getCapacity() - s.getRemaining())); -> totalUsedSpaces.add(t, s.getCapacity() - s.getRemaining());
| for(StorageReport s : r.getStorageReports()) { | ||
| if (s.getStorage().getStorageType() == t) { | ||
| capacity += s.getCapacity(); | ||
| dfsUsed += s.getDfsUsed(); | ||
| totalUsed += (s.getCapacity() - s.getRemaining()); |
| @@ -138,7 +138,7 @@ String getName() { | |||
| void accumulateSpaces(DatanodeStorageReport r) { | |||
| for(StorageReport s : r.getStorageReports()) { | |||
| final StorageType t = s.getStorage().getStorageType(); | |||
| totalCapacities.add(t, s.getCapacity()); | |||
| totalCapacities.add(t, s.getRemaining() + s.getBlockPoolUsed()); | |||
There was a problem hiding this comment.
IMO, it's difficult to understand the meaning of this improvement, just suggest add some annotations for Pool policy and describe what and why.
|
@Hexiaoqiao Thanks for your suggestion. The code style problems are fixed and some extra comments are added. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Jun 8, 2023
|
Committed to trunk. Thanks @zhangshuyan0 for your contribution! |
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
When we run balancer with
BalancingPolicy.Nodepolicy, our goal is to make each datanode storage balanced. But in the current implementation, the balancer doesn't account for storage used by non-dfs on the datanodes, which can make the situation worse for datanodes that are already strained on storage.How was this patch tested?
Add a new UT.