HDDS-6648. Fix the inconsistency between network-topology-default.xml and network-topology-default.yaml

[symious]

What changes were proposed in this pull request?

In NodeSchemaLoader.init(), the file of "network-topology-default.xml" is loaded to retrieve the topology schema.

By default the result schema is "/datacenter/rack/node", and the schema size is 3, since datacenter is treated as "ROOT".

The schema size will also be sent to NetworkTopologyImpl to build the topology, the schema size is used as maxLevel.

public NetworkTopologyImpl(ConfigurationSource conf) {
  schemaManager = NodeSchemaManager.getInstance();
  schemaManager.init(conf);
  maxLevel = schemaManager.getMaxLevel();
  factory = InnerNodeImpl.FACTORY;
  clusterTree = factory.newInnerNode(ROOT, null, null,
      NetConstants.ROOT_LEVEL,
      schemaManager.getCost(NetConstants.ROOT_LEVEL));
} 

But for "/datacenter/rack/node" the maxLevel is in fact 4, the reason of the issue is that "datacenter" is treated as "ROOT" incorrectly. While the yaml file version is correctly treated "datacenter" to a InnerNode.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6648

How was this patch tested?

unit test.

[symious]

@adoroszlai @ChenSammi Could you help to review the PR?

[ChenSammi]

@symious , the default "network-topology-default.xml" file, the total level is 3, that is / (root), rack, node. Another topology file "network-topology-nodegroup.xml" , total level is 4, including a new nodegroup level.

Could you further explain the problem a bit more? I don't quite understand the issue.

[symious]

@ChenSammi Thanks for the review.
The descriptions had some flaws, update and fixed it.

The problem I think is the content of the topology, in network-topology-default.xml, the topology is "/datacenter/rack/node", which is a 4 level topology, including "root, datacenter, rack and node", but the trick thing in "NodeSchemaLoader" is it ignores the first "/" so the schema becomes "datacenter/rack/node", which becomes a 3 level topology which fits the default setting for datanodes, that is "/default-rack/node1".

I think it's better to remove the trick operation to reduce any possible misunderstandings.

[ChenSammi]

The problem I think is the content of the topology, in network-topology-default.xml, the topology is "/datacenter/rack/node", which is a 4 level topology, including "root, datacenter, rack and node", but the trick thing in "NodeSchemaLoader" is it ignores the first "/" so the schema becomes "datacenter/rack/node", which becomes a 3 level topology which fits the default setting for datanodes, that is "/default-rack/node1".

In network-topology-default.xml, "datacenter" is just the name ID of the root layer in this topology configuration file. Please don't treat the "/" in topology configuration file the same as "/" in code.
The "/" in topology configuration file is just a separator between different layers. So the ID "datacenter" doesn't introduce a new layer. Does this answer your question?

[symious]

Thanks for the reply. @ChenSammi
Yes, I understand that the level in network-topology-default.xml is different from the meanings in code.

But the misunderstanding still exists, especially when users want to add a real level named datacenter. It might look like "/datacenter/datacenter/rack/node"

[ChenSammi]

But the misunderstanding still exists, especially when users want to add a real level named datacenter. It might look like "/datacenter/datacenter/rack/node"

If user wants to add a new level, he/she should create a new topology file, using the name preferred, or change the level ID to whatever he/she wants. "datacenter" is just a name, it can be changed.

[symious]

Indeed, the name can be changed, but this change is evitable.

In fact, I don't have any questions on the topology content of "network-topology-default.xml", I have took some time checking the code to realize that the node schema file is of different means with the action topology in code. But the confusion still exists as follows:

1. topology in network-topology-default.xml is "/datacenter/rack/node", but the actual topology in code is "/rack/node", they are different.
2. topology in network-topology-default.xml is "/datacenter/rack/node", but the topology in network-topology-default.yaml is "/root/datacenter/rack/node", they are different.
3. "datacenter" is already in topology in network-topology-default.xml, if I need to add a new level named datacenter, I need to change the name of the first "datacenter", will it cause any issues?

And I think these confusions should come to most of the users trying to work with the topology, although these are not hard to find the answers, they still need to spend some time to solve these confusions before they can work on.
IMHO, one of the benifits of open-source is reduce repeat solving problems, if one problem or confusion solved, others can take advantages of it, time can be saved to work on other features.

[ChenSammi]

2. topology in network-topology-default.xml is "/datacenter/rack/node", but the topology in network-topology-default.yaml is "/root/datacenter/rack/node", they are different.

This inconsistency is an issue. It should be fixed.

3. "datacenter" is already in topology in network-topology-default.xml, if I need to add a new level named datacenter, I need to change the name of the first "datacenter", will it cause any issues?

It should not cause any issue.

[symious]

3. "datacenter" is already in topology in network-topology-default.xml, if I need to add a new level named datacenter, I need to change the name of the first "datacenter", will it cause any issues?

It should not cause any issue.

If the name can be changed, how about change the current "datacenter" to "root"?

[ChenSammi]

If the name can be changed, how about change the current "datacenter" to "root"?

Basically, I don't suggest to change the name "datacenter" in default topology. We defined node "type" in the configuration. There are Root, Inner_Node, and Leaf type. This type matters, while layer id doesn't. Also in a Hadoop typical cluster(HDFS), user will usually think the the default root is datacenter. This user experience is consistent.

[ChenSammi]

@symious , as we have a consensus on the inconsistency between network-topology-default.xml and network-topology-default.yaml should be fixed, would you like to refactor this patch so that we can have it fixed and merged?

[symious]

@ChenSammi Sure, update the PR, please have a look.

[ChenSammi]

There is no nodegroup in default topology. We can remove this layer.

Could you add a unit test in TestYamlSchemaLoader.java? load this default topology configuration file, and check there is 3 level NodeSchema in NodeSchemaManager?

[symious]

@ChenSammi Thanks for the review. Updated the PR, please have a check.

[ChenSammi]

LGTM +1. Thanks @symious for the contribution.

[symious]

@ChenSammi Thanks for the review and merge.