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/05/25 03:50:32 UTC
[GitHub] [ozone] umamaheswararao opened a new pull request, #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
umamaheswararao opened a new pull request, #3453:
URL: https://github.com/apache/ozone/pull/3453
## What changes were proposed in this pull request?
Moved the condition before getting pipeline. We should not have sanity checks after adding pipelines into DB. If there is an issue and failed with this sanity checks, SCM can crash and it will never come back as the same issue can trigger again when loading them from db.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6795
## How was this patch tested?
Checking on existing tests.
--
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] adoroszlai commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r888220405
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -227,6 +227,16 @@ public List<DatanodeDetails> chooseDatanodes(
placementStatus.expectedPlacementCount();
throw new SCMException(errorMsg, null);
}
+ if (nodesRequiredToChoose != chosenNodes.size()) {
+ String reason = "Chosen nodes size: " + chosenNodes
+ .size() + ", but required nodes to choose: " + nodesRequiredToChoose
+ + " do not match.";
+ LOG.warn("Placement policy could not choose the enough nodes."
+ + " {} Available nodes count: {}, Excluded nodes count: {}",
+ reason, totalNodesCount, excludedNodesCount);
+ throw new SCMException(reason,
+ SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES);
+ }
Review Comment:
I wonder if `validateContainerPlacement` should verify the number of chosen nodes vs. required nodes (either in base class or this specific subclass).
--
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] umamaheswararao commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r883361645
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -194,6 +194,18 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig,
try {
Pipeline pipeline = pipelineFactory.create(replicationConfig,
excludedNodes, favoredNodes);
+ // In case in case if provided pipeline provider returns null.
Review Comment:
Moved the check into pipelineFactory#create
--
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] JacksonYao287 commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r882384077
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -194,6 +194,18 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig,
try {
Pipeline pipeline = pipelineFactory.create(replicationConfig,
excludedNodes, favoredNodes);
+ // In case in case if provided pipeline provider returns null.
Review Comment:
agree with stephen. maybe we would better to check and handle this in that certain placement policy. if the placement policy can not choose enough datanodes for the new pipeline , an exception should be thrown from it.
--
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] sodonnel commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r882068285
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java:
##########
@@ -70,14 +70,6 @@ class PipelineStateMap {
* @throws IOException if pipeline with provided pipelineID already exists
*/
void addPipeline(Pipeline pipeline) throws IOException {
- Preconditions.checkNotNull(pipeline, "Pipeline cannot be null");
Review Comment:
Do you think we should still have a check here, and at least log if the pipeline does not have enough nodes and skip it, rather than loading it? I guess we should not get a "bad pipeline" here, as it never should be allowed to be added anyway.
--
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] adoroszlai commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r888275673
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -227,6 +227,16 @@ public List<DatanodeDetails> chooseDatanodes(
placementStatus.expectedPlacementCount();
throw new SCMException(errorMsg, null);
}
+ if (nodesRequiredToChoose != chosenNodes.size()) {
+ String reason = "Chosen nodes size: " + chosenNodes
+ .size() + ", but required nodes to choose: " + nodesRequiredToChoose
+ + " do not match.";
+ LOG.warn("Placement policy could not choose the enough nodes."
+ + " {} Available nodes count: {}, Excluded nodes count: {}",
+ reason, totalNodesCount, excludedNodesCount);
+ throw new SCMException(reason,
+ SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES);
+ }
Review Comment:
Makes sense, I see the message is referencing racks.
--
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] umamaheswararao merged pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged PR #3453:
URL: https://github.com/apache/ozone/pull/3453
--
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] umamaheswararao commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r888259420
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -227,6 +227,16 @@ public List<DatanodeDetails> chooseDatanodes(
placementStatus.expectedPlacementCount();
throw new SCMException(errorMsg, null);
}
+ if (nodesRequiredToChoose != chosenNodes.size()) {
+ String reason = "Chosen nodes size: " + chosenNodes
+ .size() + ", but required nodes to choose: " + nodesRequiredToChoose
+ + " do not match.";
+ LOG.warn("Placement policy could not choose the enough nodes."
+ + " {} Available nodes count: {}, Excluded nodes count: {}",
+ reason, totalNodesCount, excludedNodesCount);
+ throw new SCMException(reason,
+ SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES);
+ }
Review Comment:
I assumed that is more validating the placement with respective to racks. Some how we are still seeing the issue of getting less nodes than requested.
--
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] sodonnel commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r882069865
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -194,6 +194,18 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig,
try {
Pipeline pipeline = pipelineFactory.create(replicationConfig,
excludedNodes, favoredNodes);
+ // In case in case if provided pipeline provider returns null.
Review Comment:
As this change is required due what we suspect is a bug in the RackScatterPlacementPolicy, where it returns less nodes than expected without giving an exception, I think we should make a change to the RackScatter policy too, so that it throws an exception indicating it is about to return successfully with less than expected. You could add a reference to this Jira in a comment so we can remove it later if we get to the bottom of the suspected bug.
--
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] umamaheswararao commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r882102833
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java:
##########
@@ -70,14 +70,6 @@ class PipelineStateMap {
* @throws IOException if pipeline with provided pipelineID already exists
*/
void addPipeline(Pipeline pipeline) throws IOException {
- Preconditions.checkNotNull(pipeline, "Pipeline cannot be null");
Review Comment:
I think it make sense to leave this preconditions as is. Anyway we are making sure to pass the correct pipeline with other checks.
--
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] umamaheswararao commented on a diff in pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3453:
URL: https://github.com/apache/ozone/pull/3453#discussion_r882103255
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -194,6 +194,18 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig,
try {
Pipeline pipeline = pipelineFactory.create(replicationConfig,
excludedNodes, favoredNodes);
+ // In case in case if provided pipeline provider returns null.
Review Comment:
oh, yeah I remember we talked about this. Let me add that.
--
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] umamaheswararao commented on pull request #3453: HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3453:
URL: https://github.com/apache/ozone/pull/3453#issuecomment-1145214077
Thanks @adoroszlai for the review
--
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