errose28 commented on code in PR #11861:
URL: https://github.com/apache/ozone/pull/11861#discussion_r4450615376
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -328,6 +328,12 @@ private ContainerWithPipeline
getContainerWithPipelineCommon(
pipeline = null;
}
+ // An OPEN pipeline reloaded from the SCM DB can list datanodes that have
not registered since SCM started.
+ // Build the pipeline from the replicas instead, which only include
datanodes that have reported.
+ if (pipeline != null &&
scm.getPipelineManager().isOpenWithUnregisteredNodes(pipeline)) {
+ pipeline = null;
+ }
Review Comment:
Recon's `ReconContainerManager#addNewContainer` will also hit this path. It
will cause Recon to record a random read pipeline ID as the write pipeline
associated with the open container. Recon should probably be fixed as well to
use a pipeline info RPC with the pipeline ID from the `ContainerInfo` object to
get the real membership.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableRatisContainerProvider.java:
##########
@@ -149,8 +149,11 @@ private ContainerInfo getContainer(ReplicationConfig
repConfig, String owner,
// mentioned in HDDS-5655.
pipelineManager.acquireReadLock();
try {
+ // Skip pipelines with datanodes that have not registered since SCM
started, they are not safe for writes.
List<Pipeline> availablePipelines = findPipelinesByState(repConfig,
- excludeList, Pipeline.PipelineState.OPEN);
+ excludeList, Pipeline.PipelineState.OPEN).stream()
+ .filter(p -> !pipelineManager.isOpenWithUnregisteredNodes(p))
Review Comment:
This adds another `O(pipeline)` pass through all the pipelines on write. We
can put this check inside the existing loop in `selectContainer` to avoid any
extra overhead.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -651,12 +651,8 @@ public void scrubPipelines() throws IOException {
}
}
- /**
- * @param pipeline The pipeline to check
- * @return True if the pipeline is open and contains unregistered nodes.
False
- * otherwise.
- */
- private boolean isOpenWithUnregisteredNodes(Pipeline pipeline) {
+ @Override
+ public boolean isOpenWithUnregisteredNodes(Pipeline pipeline) {
if (!pipeline.isOpen()) {
return false;
}
Review Comment:
Dormant pipelines (from `ozone admin pipeline deactivate`) will contain open
containers and need to service reads. It's probably safer to filter out all
SCM-persisted pipelines with unregistered nodes regardless of their state.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableRatisContainerProvider.java:
##########
@@ -149,8 +149,11 @@ private ContainerInfo getContainer(ReplicationConfig
repConfig, String owner,
// mentioned in HDDS-5655.
pipelineManager.acquireReadLock();
try {
+ // Skip pipelines with datanodes that have not registered since SCM
started, they are not safe for writes.
Review Comment:
Similar to the EC provider, I think we need a unique log when we can't serve
the write due to unregistered members.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -113,8 +114,11 @@ public ContainerInfo getContainer(final long size,
excludeList);
}
}
+ // Skip pipelines with datanodes that have not registered since SCM
started, they are not safe for writes.
List<Pipeline> existingPipelines = pipelineManager.getPipelines(
- repConfig, Pipeline.PipelineState.OPEN);
+ repConfig, Pipeline.PipelineState.OPEN).stream()
+ .filter(p -> !pipelineManager.isOpenWithUnregisteredNodes(p))
Review Comment:
Similar to the ratis provider, if we move the filter inside the while loop
on line 129 we will not add `O(pipeline)` overhead to each request.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -113,8 +114,11 @@ public ContainerInfo getContainer(final long size,
excludeList);
}
}
+ // Skip pipelines with datanodes that have not registered since SCM
started, they are not safe for writes.
Review Comment:
The `openPipelineCount` variable used to check the pipeline limit includes
pipelines which are currently unusable due to unregistered members. This makes
the existing log messages somewhat misleading. We should log a unique message
if we cannot serve an EC write because the pipeline limit is reached and all
pipelines have an unregistered member.
We may also want to check our safemode rules in a follow-up change since I
don't think there is a check that we have at least one usable EC pipeline.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
