Yiqun Lin commented on HDFS-13418:

Thanks [~Tao Jie] for updating the patch. Almost looks good, some comments from 
 * Can we define a var {{DFS_NET_TOPOLOGY_IMPL_DEFAULT}} as the default 
NetworkTopology impl class used in HDFS rather than hard-coded in 
 * For the description of {{dfs.net.topology.impl}}, {{The implementation class 
of NetworkTopology used in block placement.}} can be updated to {{The 
implementation class of NetworkTopology used in HDFS.}} And then adding new one 
sentence to introduce the default impl class: {{By default, the class 
org.apache.hadoop.hdfs.net.DFSNetworkTopology is specified and used in block 

For the unit test, seems we don't need to extend the new NetworkTopology to 
test. Current existing {{NetworkTopology}} impls are enough. I think there will 
be three cases we need to cover.
 * Switch {{dfs.use.dfs.network.topology}} to true, then verify if the default 
DFSNetworkTopology impl is got.
 * Switch {{dfs.use.dfs.network.topology}} to false, then verify if the default 
NetworkTopology impl is got.
 * Switch {{dfs.use.dfs.network.topology}} to false, and change the value of 
{{net.topology.impl}}, then verify if the desired NetworkTopology impl is got.

>  NetworkTopology should be configurable when enable DFSNetworkTopology
> ----------------------------------------------------------------------
>                 Key: HDFS-13418
>                 URL: https://issues.apache.org/jira/browse/HDFS-13418
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>    Affects Versions: 3.0.1
>            Reporter: Tao Jie
>            Assignee: Tao Jie
>            Priority: Major
>         Attachments: HDFS-13418.001.patch, HDFS-13418.002.patch
> In HDFS-11530 we introduce DFSNetworkTopology and in HDFS-11998 we set 
> DFSNetworkTopology as the default implementation.
> We still have {{net.topology.impl=org.apache.hadoop.net.NetworkTopology}} in 
> core-site.default. Actually this property does not effect once 
> {{dfs.use.dfs.network.topology}} is true. 
> in {{DatanodeManager}},networkTopology is initialized as 
> {code}
>     if (useDfsNetworkTopology) {
>       networktopology = DFSNetworkTopology.getInstance(conf);
>     } else {
>       networktopology = NetworkTopology.getInstance(conf);
>     }
> {code}
> I think we should still make the NetworkTopology  configurable rather than 
> hard code the implementation since we may need another NetworkTopology impl.
> I am not sure if there is other consideration. Any thought? [~vagarychen] 
> [~linyiqun]

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org

Reply via email to