You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sodonnel (via GitHub)" <gi...@apache.org> on 2023/06/23 16:52:48 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #4968: HDDS-8919. Allow a pipeline to be created and then added to PipelineManager in two steps
sodonnel opened a new pull request, #4968:
URL: https://github.com/apache/ozone/pull/4968
## What changes were proposed in this pull request?
As it stands, the method:
```
PipelineManager.createPipeline(ReplicationConfig replicationConfig,
List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes)
```
Is used to create a new pipeline and add it to the Pipeline manager. This creation and adding is all performed under a single lock The somewhat expensive node selection process from the placement policy is performed under the lock.
For Ratis pipelines, this could be important, as pipelines are created and limited based on the number of existing pipelines on the nodes.
EC pipelines do not have such a limitation, and they also need to be created much more frequently due to their short lived nature.
This PR changes the pipeline manager to have a buildPipeline Method, which calls the placement policy and creates the new pipeline object without any locks.
Then there is an addPipeline method to add the created pipeline to the pipelineManager under the lock.
This will allow for the WritableECPipelineProvider to build pipelines without blocking other threads and then add them to the manager under a lock, reducing the amount of work performed under a lock. This change would need to be made in another PR.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8919
## How was this patch tested?
Modified one test to call the new methods
--
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] guohao-rosicky commented on pull request #4968: HDDS-8919. Allow a pipeline to be created and then added to PipelineManager in two steps
Posted by "guohao-rosicky (via GitHub)" <gi...@apache.org>.
guohao-rosicky commented on PR #4968:
URL: https://github.com/apache/ozone/pull/4968#issuecomment-1605936465
Thanks @sodonnel work on this. This will help the performance of the ec container allocate block.
--
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 #4968: HDDS-8919. Allow a pipeline to be created and then added to PipelineManager in two steps
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4968:
URL: https://github.com/apache/ozone/pull/4968#discussion_r1242004822
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -219,25 +274,24 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig,
LOG.info(message);
throw new IOException(message);
}
+ }
+ private void addPipelineToManager(Pipeline pipeline)
+ throws IOException, TimeoutException {
acquireWriteLock();
- final Pipeline pipeline;
try {
- pipeline = pipelineFactory.create(replicationConfig,
- excludedNodes, favoredNodes);
stateManager.addPipeline(pipeline.getProtobufMessage(
ClientVersion.CURRENT_VERSION));
Review Comment:
`pipeline.getProtobufMessage` can now be moved outside the locked section.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -219,25 +274,24 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig,
LOG.info(message);
throw new IOException(message);
}
+ }
+ private void addPipelineToManager(Pipeline pipeline)
+ throws IOException, TimeoutException {
acquireWriteLock();
- final Pipeline pipeline;
try {
- pipeline = pipelineFactory.create(replicationConfig,
- excludedNodes, favoredNodes);
stateManager.addPipeline(pipeline.getProtobufMessage(
ClientVersion.CURRENT_VERSION));
} catch (IOException | TimeoutException ex) {
- LOG.debug("Failed to create pipeline with replicationConfig {}.",
- replicationConfig, ex);
+ LOG.debug("Failed to add pipeline with replicationConfig {}.",
+ pipeline.getReplicationConfig(), ex);
Review Comment:
Nit: we may want to log the pipeline itself. Previously this was called with a fresh pipeline, but now `pipeline` can be "anything".
--
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 merged pull request #4968: HDDS-8919. Allow EC pipelines to be created and then added to PipelineManager in two steps
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4968:
URL: https://github.com/apache/ozone/pull/4968
--
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 pull request #4968: HDDS-8919. Allow a pipeline to be created and then added to PipelineManager in two steps
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4968:
URL: https://github.com/apache/ozone/pull/4968#issuecomment-1607855136
@sodonnel Thanks for updating the PR. It seems there is a test for `PipelineManagerImpl` failing due to the new replication type check:
```
Tests run: 22, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 13.469 s <<< FAILURE! - in org.apache.hadoop.hdds.scm.pipeline.TestPipelineManagerImpl
[ERROR] org.apache.hadoop.hdds.scm.pipeline.TestPipelineManagerImpl.testCreatePipeline Time elapsed: 0.431 s <<< ERROR!
java.lang.IllegalArgumentException: Replication type must be EC
at org.apache.hadoop.hdds.scm.pipeline.PipelineManagerImpl.buildECPipeline(PipelineManagerImpl.java:214)
at org.apache.hadoop.hdds.scm.pipeline.TestPipelineManagerImpl.testCreatePipeline(TestPipelineManagerImpl.java:199)
```
--
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 pull request #4968: HDDS-8919. Allow a pipeline to be created and then added to PipelineManager in two steps
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #4968:
URL: https://github.com/apache/ozone/pull/4968#issuecomment-1604555171
There are two things which concern me about this change:
1. It is fine for EC pipeline to be built and never added to the manager, but for Ratis, it is not. Building a Ratis pipeline actually sends messages to the DNs to establish the Ratis ring. Therefore it might make sense to throw an error in a call comes into BuildPipeline with a Ratis replicationConfig.
2. I was worried about a race condition where we create a pipeline, and then some of the nodes go stale or enter some maintenance state. Both of those events trigger the closing of the pipelines on the affect nodes via the StaleNodeHandler and the StartDatanodeAdminHandler. In both cases, these handlers get the pipeline list on the datanode from NodeManager, which stores it in a concurrent hash map. So even before this change, there is a chance that a pipeline is created, right as the node state changes, and the staleNodeHandler or StartDatanodeAdminHandler will miss the pipeline as it has not made it into the nodeManager map yet. For stale nodes, it would get caught when it goes dead. For decommission / maintenance, I believe it would be a problem (this problem already exists in theory and is no worse due to this change) as the DatanodeAdminMonitorImpl would keep waiting until the pipelines get closed. It would make sense to do something in DatanodeAdminMonitorImpl to trigger ano
ther close if it finds pipelines still open.
--
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