You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/26 08:07:51 UTC
[GitHub] [ozone] symious opened a new pull request, #3350: HDDS-6648. Add Root in network-topology-default.xml
symious opened a new pull request, #3350:
URL: https://github.com/apache/ozone/pull/3350
## 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/nodegroup/node", and the schema size is 4, 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/nodegroup/node" the maxLevel is in fact 5, 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1119208925
> > 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"?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #3350: HDDS-6648. Fix the inconsistency between network-topology-default.xml and network-topology-default.yaml
Posted by GitBox <gi...@apache.org>.
ChenSammi merged PR #3350:
URL: https://github.com/apache/ozone/pull/3350
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1119200294
> 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Fix the inconsistency between network-topology-default.xml and network-topology-default.yaml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1135593823
@ChenSammi Thanks for the review and merge.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1121795837
> 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1131686223
@ChenSammi Sure, update the PR, please have a look.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1109485098
@adoroszlai @ChenSammi Could you help to review the PR?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1118131726
@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 get the point.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3350:
URL: https://github.com/apache/ozone/pull/3350#discussion_r877842498
##########
hadoop-hdds/common/src/main/resources/network-topology-default.yaml:
##########
@@ -30,32 +30,26 @@ prefix: /
type: ROOT
# Layer name
-defaultName: root
+defaultName: datacenter
# Sub layer
# The sub layer property defines as a list which can reflect a node tree, though
# in schema template it always has only one child.
sublayer:
-
cost: 1
- prefix: dc
- defaultName: datacenter
+ prefix: rack
+ defaultName: rack
type: INNER_NODE
sublayer:
- -
- cost: 1
- prefix: rack
- defaultName: rack
- type: INNER_NODE
- sublayer:
+ -
+ cost: 1
+ prefix: ng
+ defaultName: nodegroup
Review Comment:
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?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1131625940
@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?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1118134680
@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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on a diff in pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on code in PR #3350:
URL: https://github.com/apache/ozone/pull/3350#discussion_r877899377
##########
hadoop-hdds/common/src/main/resources/network-topology-default.yaml:
##########
@@ -30,32 +30,26 @@ prefix: /
type: ROOT
# Layer name
-defaultName: root
+defaultName: datacenter
# Sub layer
# The sub layer property defines as a list which can reflect a node tree, though
# in schema template it always has only one child.
sublayer:
-
cost: 1
- prefix: dc
- defaultName: datacenter
+ prefix: rack
+ defaultName: rack
type: INNER_NODE
sublayer:
- -
- cost: 1
- prefix: rack
- defaultName: rack
- type: INNER_NODE
- sublayer:
+ -
+ cost: 1
+ prefix: ng
+ defaultName: nodegroup
Review Comment:
@ChenSammi Thanks for the review. Updated the PR, please have a check.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1135562731
LGTM +1. Thanks @symious for the contribution.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1118280676
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"
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1118354578
> 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 want 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 want. "datacenter" is just a name, it can be changed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1119141075
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3350: HDDS-6648. Add Root in network-topology-default.xml
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3350:
URL: https://github.com/apache/ozone/pull/3350#issuecomment-1118247547
> 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?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org