[
https://issues.apache.org/jira/browse/HDFS-17901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18075062#comment-18075062
]
ASF GitHub Bot commented on HDFS-17901:
---------------------------------------
kokonguyen191 commented on code in PR #8386:
URL: https://github.com/apache/hadoop/pull/8386#discussion_r3115703263
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/net/DFSNetworkTopology.java:
##########
@@ -385,6 +385,14 @@ private ArrayList<DFSTopologyNodeImpl> getEligibleChildren(
} else if (excludedNode instanceof DFSTopologyNodeImpl) {
storageCount -= ((DFSTopologyNodeImpl) excludedNode)
.getSubtreeStorageCount(type);
+ } else if (excludedNode instanceof DatanodeInfo) {
+ String nodeLocation = excludedNode.getNetworkLocation()
+ + "/" + excludedNode.getName();
+ DatanodeDescriptor dn =
(DatanodeDescriptor)getNode(nodeLocation);
Review Comment:
Nit: `DatanodeDescriptor dn = (DatanodeDescriptor) getNode(nodeLocation`
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/net/DFSNetworkTopology.java:
##########
@@ -385,6 +385,14 @@ private ArrayList<DFSTopologyNodeImpl> getEligibleChildren(
} else if (excludedNode instanceof DFSTopologyNodeImpl) {
storageCount -= ((DFSTopologyNodeImpl) excludedNode)
.getSubtreeStorageCount(type);
+ } else if (excludedNode instanceof DatanodeInfo) {
+ String nodeLocation = excludedNode.getNetworkLocation()
+ + "/" + excludedNode.getName();
Review Comment:
Maybe can use `NetworkTopology#PATH_SEPARATOR_STR` instead of magic string
`"/"`?
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/net/DFSNetworkTopology.java:
##########
@@ -385,6 +385,14 @@ private ArrayList<DFSTopologyNodeImpl> getEligibleChildren(
} else if (excludedNode instanceof DFSTopologyNodeImpl) {
storageCount -= ((DFSTopologyNodeImpl) excludedNode)
.getSubtreeStorageCount(type);
+ } else if (excludedNode instanceof DatanodeInfo) {
+ String nodeLocation = excludedNode.getNetworkLocation()
+ + "/" + excludedNode.getName();
+ DatanodeDescriptor dn =
(DatanodeDescriptor)getNode(nodeLocation);
+ if (dn == null) {
+ continue;
+ }
+ storageCount -= dn.hasStorageType(type)? 1 : 0;
Review Comment:
Nit: `storageCount -= dn.hasStorageType(type) ? 1 : 0;`
> Fix DFSNetworkTopology#getEligibleChildren missing DatanodeInfo handling
> causing incorrect storageCount
> -------------------------------------------------------------------------------------------------------
>
> Key: HDFS-17901
> URL: https://issues.apache.org/jira/browse/HDFS-17901
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: hdfs
> Affects Versions: 3.4.3
> Reporter: yue.wang
> Assignee: Yue Wang
> Priority: Major
> Labels: pull-request-available
>
> In DFSNetworkTopology#getEligibleChildren(), when iterating over
> excludedNodes, the method only handles DatanodeDescriptor and
> DFSTopologyNodeImpl types but misses the DatanodeInfo type. This causes
> storageCount to be incorrectly calculated (overestimated), leading to
> inconsistency with the parent method chooseRandomWithStorageType() and
> potentially incorrect weighted random node selection.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
