[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r398622023
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -403,7 +408,7 @@ private boolean checkAllNodesAreEqual(NetworkTopology
topology) {
@VisibleForTesting
protected DatanodeDetails chooseNodeFromNetworkTopology(
NetworkTopology networkTopology, DatanodeDetails anchor,
- List excludedNodes) {
+ List excludedNodes) throws SCMException {
Review comment:
Updated.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r398621936
##
File path:
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelinePlacementPolicy.java
##
@@ -43,36 +47,98 @@
private MockNodeManager nodeManager;
private OzoneConfiguration conf;
private PipelinePlacementPolicy placementPolicy;
+ private NetworkTopologyImpl cluster;
private static final int PIPELINE_PLACEMENT_MAX_NODES_COUNT = 10;
+ private List nodesWithOutRackAwareness = new ArrayList<>();
+ private List nodesWithRackAwareness = new ArrayList<>();
+
@Before
public void init() throws Exception {
-nodeManager = new MockNodeManager(true,
-PIPELINE_PLACEMENT_MAX_NODES_COUNT);
+cluster = initTopology();
+// start with nodes with rack awareness.
+nodeManager = new MockNodeManager(cluster, getNodesWithRackAwareness(),
+false, PIPELINE_PLACEMENT_MAX_NODES_COUNT);
conf = new OzoneConfiguration();
conf.setInt(OZONE_DATANODE_PIPELINE_LIMIT, 5);
placementPolicy = new PipelinePlacementPolicy(
nodeManager, new PipelineStateManager(), conf);
}
+ private NetworkTopologyImpl initTopology() {
+NodeSchema[] schemas = new NodeSchema[]
+{ROOT_SCHEMA, RACK_SCHEMA, LEAF_SCHEMA};
+NodeSchemaManager.getInstance().init(schemas, true);
+NetworkTopologyImpl topology =
+new NetworkTopologyImpl(NodeSchemaManager.getInstance());
+return topology;
+ }
+
+ private List getNodesWithRackAwareness() {
+List datanodes = new ArrayList<>();
+for (Node node : NODES) {
+ DatanodeDetails datanode = overwriteLocationInNode(
+ getNodesWithoutRackAwareness(), node);
+ nodesWithRackAwareness.add(datanode);
+ datanodes.add(datanode);
+}
+return datanodes;
+ }
+
+ private DatanodeDetails getNodesWithoutRackAwareness() {
+DatanodeDetails node = MockDatanodeDetails.randomDatanodeDetails();
+nodesWithOutRackAwareness.add(node);
+return node;
+ }
+
@Test
- public void testChooseNodeBasedOnNetworkTopology() {
-List healthyNodes =
-nodeManager.getNodes(HddsProtos.NodeState.HEALTHY);
-DatanodeDetails anchor = placementPolicy.chooseNode(healthyNodes);
+ public void testChooseNodeBasedOnNetworkTopology() throws SCMException {
+DatanodeDetails anchor =
placementPolicy.chooseNode(nodesWithRackAwareness);
// anchor should be removed from healthyNodes after being chosen.
-Assert.assertFalse(healthyNodes.contains(anchor));
+Assert.assertFalse(nodesWithRackAwareness.contains(anchor));
List excludedNodes =
new ArrayList<>(PIPELINE_PLACEMENT_MAX_NODES_COUNT);
excludedNodes.add(anchor);
DatanodeDetails nextNode = placementPolicy.chooseNodeFromNetworkTopology(
nodeManager.getClusterNetworkTopologyMap(), anchor, excludedNodes);
Assert.assertFalse(excludedNodes.contains(nextNode));
-// nextNode should not be the same as anchor.
+// next node should not be the same as anchor.
Assert.assertTrue(anchor.getUuid() != nextNode.getUuid());
+// next node should be on the same rack based on topology.
+Assert.assertEquals(anchor.getNetworkLocation(),
+nextNode.getNetworkLocation());
}
+ @Test
+ public void testChooseNodeWithSingleNodeRack() throws SCMException {
+// There is only one node on 3 racks altogether.
+List datanodes = new ArrayList<>();
+for (Node node : SINGLE_NODE_RACK) {
+ DatanodeDetails datanode = overwriteLocationInNode(
+ MockDatanodeDetails.randomDatanodeDetails(), node);
+ datanodes.add(datanode);
+}
+MockNodeManager localNodeManager = new MockNodeManager(null, datanodes,
Review comment:
You are right. I updated this part. Thanks!
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r397830892 ## File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelinePlacementPolicy.java ## @@ -21,6 +21,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdds.conf.OzoneConfiguration; Review comment: I just added a test for this case. Please see the updates. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r397795926
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -414,6 +416,12 @@ protected DatanodeDetails chooseNodeFromNetworkTopology(
Node pick = networkTopology.chooseRandom(
anchor.getNetworkLocation(), excluded);
DatanodeDetails pickedNode = (DatanodeDetails) pick;
+if (pickedNode == null) {
+ LOG.debug("Pick node is null, excluded nodes {}, anchor {}.",
+ excluded, anchor);
+ throw new SCMException("Unable to find node based on Topology.",
+ SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE);
+}
Review comment:
Updated.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r397795809
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -99,9 +99,8 @@ boolean meetCriteria(DatanodeDetails datanodeDetails, int
nodesRequired) {
try {
pipeline = stateManager.getPipeline(pid);
} catch (PipelineNotFoundException e) {
-LOG.error("Pipeline not found in pipeline state manager during" +
-" pipeline creation. PipelineID: " + pid +
-" exception: " + e.getMessage());
+LOG.debug("Pipeline not found in pipeline state manager during" +
+" pipeline creation. PipelineID: {}" + pid + e);
Review comment:
Updated.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r397795529
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -292,10 +294,15 @@ DatanodeDetails fallBackPickNodes(
int nodesToFind = nodesRequired - results.size();
for (int x = 0; x < nodesToFind; x++) {
// Pick remaining nodes based on the existence of rack awareness.
- DatanodeDetails pick = rackAwareness
- ? chooseNodeFromNetworkTopology(
- nodeManager.getClusterNetworkTopologyMap(), anchor, exclude)
- : fallBackPickNodes(healthyNodes, exclude);
+ DatanodeDetails pick;
+ try {
+pick = rackAwareness
+? chooseNodeFromNetworkTopology(
+nodeManager.getClusterNetworkTopologyMap(), anchor, exclude)
+: fallBackPickNodes(healthyNodes, exclude);
+ } catch (SCMException e) {
Review comment:
This is clean. I will make updates. Thanks for the suggestion.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r394085862
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -99,9 +99,11 @@ boolean meetCriteria(DatanodeDetails datanodeDetails, int
nodesRequired) {
try {
pipeline = stateManager.getPipeline(pid);
} catch (PipelineNotFoundException e) {
-LOG.error("Pipeline not found in pipeline state manager during" +
-" pipeline creation. PipelineID: " + pid +
-" exception: " + e.getMessage());
+if (LOG.isDebugEnabled()) {
Review comment:
Yea just updated. Thanks.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r393622294
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -99,9 +99,11 @@ boolean meetCriteria(DatanodeDetails datanodeDetails, int
nodesRequired) {
try {
pipeline = stateManager.getPipeline(pid);
} catch (PipelineNotFoundException e) {
-LOG.error("Pipeline not found in pipeline state manager during" +
-" pipeline creation. PipelineID: " + pid +
-" exception: " + e.getMessage());
+if (LOG.isDebugEnabled()) {
Review comment:
I added {} and removed isDebugEnabled 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r393621978
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -292,10 +294,19 @@ DatanodeDetails fallBackPickNodes(
int nodesToFind = nodesRequired - results.size();
for (int x = 0; x < nodesToFind; x++) {
// Pick remaining nodes based on the existence of rack awareness.
- DatanodeDetails pick = rackAwareness
- ? chooseNodeFromNetworkTopology(
- nodeManager.getClusterNetworkTopologyMap(), anchor, exclude)
- : fallBackPickNodes(healthyNodes, exclude);
+ DatanodeDetails pick;
+ try {
+pick = rackAwareness
+? chooseNodeFromNetworkTopology(
+nodeManager.getClusterNetworkTopologyMap(), anchor, exclude)
+: fallBackPickNodes(healthyNodes, exclude);
+ } catch (SCMException e) {
+pick = fallBackPickNodes(healthyNodes, exclude);
+if (LOG.isDebugEnabled()) {
Review comment:
Updated.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r393621927
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -414,6 +425,14 @@ protected DatanodeDetails chooseNodeFromNetworkTopology(
Node pick = networkTopology.chooseRandom(
anchor.getNetworkLocation(), excluded);
DatanodeDetails pickedNode = (DatanodeDetails) pick;
+if (pickedNode == null) {
+ if (LOG.isDebugEnabled()) {
Review comment:
Updated.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r393411067
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -99,9 +99,11 @@ boolean meetCriteria(DatanodeDetails datanodeDetails, int
nodesRequired) {
try {
pipeline = stateManager.getPipeline(pid);
} catch (PipelineNotFoundException e) {
-LOG.error("Pipeline not found in pipeline state manager during" +
-" pipeline creation. PipelineID: " + pid +
-" exception: " + e.getMessage());
+if (LOG.isDebugEnabled()) {
Review comment:
Thanks for the info. How about the logs without "{}" ? Shall we check if
debug is enabled?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r393411067
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -99,9 +99,11 @@ boolean meetCriteria(DatanodeDetails datanodeDetails, int
nodesRequired) {
try {
pipeline = stateManager.getPipeline(pid);
} catch (PipelineNotFoundException e) {
-LOG.error("Pipeline not found in pipeline state manager during" +
-" pipeline creation. PipelineID: " + pid +
-" exception: " + e.getMessage());
+if (LOG.isDebugEnabled()) {
Review comment:
Thanks for the info. How about the logs without "{}" ? Shall we check if
debug is enabled? @sodonnel
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline placement based on Topology does not have fallback
timmylicheng commented on a change in pull request #678: HDDS-3179 Pipeline
placement based on Topology does not have fallback
URL: https://github.com/apache/hadoop-ozone/pull/678#discussion_r392677663
##
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##
@@ -292,10 +294,15 @@ DatanodeDetails fallBackPickNodes(
int nodesToFind = nodesRequired - results.size();
for (int x = 0; x < nodesToFind; x++) {
// Pick remaining nodes based on the existence of rack awareness.
- DatanodeDetails pick = rackAwareness
- ? chooseNodeFromNetworkTopology(
- nodeManager.getClusterNetworkTopologyMap(), anchor, exclude)
- : fallBackPickNodes(healthyNodes, exclude);
+ DatanodeDetails pick;
+ try {
+pick = rackAwareness
+? chooseNodeFromNetworkTopology(
+nodeManager.getClusterNetworkTopologyMap(), anchor, exclude)
+: fallBackPickNodes(healthyNodes, exclude);
+ } catch (SCMException e) {
Review comment:
Debug log makes sense.
My thought was: fall back will happen every time when there is only one node
on the rack. Under all other scenarios, this fallback logic shouldn't take
effects. As a known case, I was reluctant to add logs for it regardless of its
log level. But I guess it can be valuable if fallback logic takes effect in
other cases.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
