virajjasani commented on a change in pull request #3714:
URL: https://github.com/apache/hadoop/pull/3714#discussion_r768730257
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
##########
@@ -2172,8 +2172,9 @@ public String getSlowDisksReport() {
.size()];
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());
+ DatanodeInfo dnInfo = new DatanodeInfoBuilder().setFrom(d).build();
+ dnInfo.setNumBlocks(d.numBlocks());
Review 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;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
