HDFS-16352. return the real datanode numBlocks in #getDatanodeStorage… #3714
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. |
|
@liubingxing Any chance we can add a test? |
liubingxing
force-pushed
the
HDFS-16352
branch
from
1299cc2 to
bed5ed4
Compare
|
@virajjasani I add UT |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
approved these changes
Dec 10, 2021
|
Sorry somehow I missed QA result from Jenkins. @liubingxing could you take care of checkstyle warning? |
liubingxing
force-pushed
the
HDFS-16352
branch
from
bed5ed4 to
81d01ff
Compare
|
@virajjasani Thanks for your review. I have fixed the checkstyle warning and updated the PR. |
Hexiaoqiao
reviewed
Dec 11, 2021
There was a problem hiding this comment.
Thanks @liubingxing for your PR. Leave one comment inline. Thanks.
There was a problem hiding this comment.
new DatanodeInfoBuilder().setFrom(d).build() has set numBlocks. Any difference to set it again here? Any other reason?
There was a problem hiding this comment.
@Hexiaoqiao new DatanodeInfoBuilder().setFrom(d).build() set the DatanodeInfo from DatanodeDescriptor which from final List<DatanodeDescriptor> datanodes = getDatanodeListForReport(type); . But DatanodeDescriptor does not set the actual numBlocks of datanode.
datanodeReport set the numBlocks like this.
There was a problem hiding this comment.
I believe we should remove this.numBlocks = from.getNumBlocks() from DatanodeInfoBuilder#setFrom(DatanodeInfo) method because even FSNamesystem#datanodeReport is also explicitly setting numBlocks:
arr[i] = new DatanodeInfoBuilder().setFrom(results.get(i))
.build();
arr[i].setNumBlocks(results.get(i).numBlocks());
And we can also add a comment in setFrom(DatanodeInfo) method stating that: use setNumBlocks specifically to set numBlocks as this method won't set it correctly.
How about this diff?
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeInfo.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeInfo.java
index bba90a05794..fbe6bcc4629 100644
--- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeInfo.java
+++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeInfo.java
@@ -698,9 +698,10 @@ public void setSoftwareVersion(String softwareVersion) {
private long nonDfsUsed = 0L;
private long lastBlockReportTime = 0L;
private long lastBlockReportMonotonic = 0L;
- private int numBlocks;
-
+ private int numBlocks = 0;
+ // Please use setNumBlocks explicitly to set numBlocks as this method doesn't have
+ // sufficient info about numBlocks
public DatanodeInfoBuilder setFrom(DatanodeInfo from) {
this.capacity = from.getCapacity();
this.dfsUsed = from.getDfsUsed();
@@ -717,7 +718,6 @@ public DatanodeInfoBuilder setFrom(DatanodeInfo from) {
this.upgradeDomain = from.getUpgradeDomain();
this.lastBlockReportTime = from.getLastBlockReportTime();
this.lastBlockReportMonotonic = from.getLastBlockReportMonotonic();
- this.numBlocks = from.getNumBlocks();
setNodeID(from);
return this;
}
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
index ef51c6ca074..cfb1d83ec5b 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
@@ -2182,7 +2182,8 @@ public String getSlowDisksReport() {
for (int i = 0; i < reports.length; i++) {
final DatanodeDescriptor d = datanodes.get(i);
reports[i] = new DatanodeStorageReport(
- new DatanodeInfoBuilder().setFrom(d).build(), d.getStorageReports());
+ new DatanodeInfoBuilder().setFrom(d).setNumBlocks(d.numBlocks()).build(),
+ d.getStorageReports());
}
return reports;
}
There was a problem hiding this comment.
Thanks @liubingxing for your replay. It makes sense to me.
|
🎊 +1 overall
This message was automatically generated. |
liubingxing
force-pushed
the
HDFS-16352
branch
from
81d01ff to
450a62a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
@virajjasani Thank you for your advice. I update the PR, please take a look. |
|
@Hexiaoqiao Please take a look. |
|
LGTM. +1 from my side. I would like to commit this improvement if no more other comments. |
|
+1 (non-binding) from my side, thanks @liubingxing and @Hexiaoqiao |
|
Thanks for the review @virajjasani @Hexiaoqiao |
Hexiaoqiao
approved these changes
Dec 17, 2021
hotcodemacha
pushed a commit
to hotcodemacha/hadoop
that referenced
this pull request
Dec 22, 2021
…Report (apache#3714). Contributed by liubingxing. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
bogthe
pushed a commit
to bogthe/hadoop
that referenced
this pull request
Feb 2, 2022
…Report (apache#3714). Contributed by liubingxing. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org> (cherry picked from commit d8dea6f)
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…Report (apache#3714). Contributed by liubingxing. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
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.
JIRA: HDFS-16352
#getDatanodeStorageReport will return the array of DatanodeStorageReport which contains the DatanodeInfo in each DatanodeStorageReport, but the numBlocks in DatanodeInfo is always zero, which is confusing
Or we can return the real numBlocks in DatanodeInfo when we call #getDatanodeStorageReport