You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ibrusentsev (via GitHub)" <gi...@apache.org> on 2023/10/09 10:03:24 UTC
[PR] HDDS-9405: Make PipelineStateMap thread-safe [ozone]
ibrusentsev opened a new pull request, #5410:
URL: https://github.com/apache/ozone/pull/5410
## What changes were proposed in this pull request?
Change PipelineStateMap to thread-safe in order to improve performance of SCM.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9405
## How was this patch tested?
Public interface of PipelineStateMap covered with unit 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
Re: [PR] HDDS-9405. Make PipelineStateMap thread-safe [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1755842278
I think we have a problem with EC for sure, but Ratis pre-creates all its pipelines up front, so it should not have a "pipeline creation" or "pipeline closing" problem at run time. It might have a container allocation problem, but I am not sure.
If we are having to allocate containers too quickly, it also suggest we have too few pipelines and too few containers per pipeline (for Ratis).
If the slowest part is the call to Ratis, and we can only send one thing to Ratis at a time, then making pipeline manager lock free is not going to help, as it will all stack up at the Ratis call. So if your profiles are showing the call to Ratis to replicate the SCM changes across the SCM, then perhaps we should focus on Ratis to see if it can be made faster.
It would be interesting to see how the throughput looks for Ratis writes with a sufficient number of pipelines as it should have a different profile than EC.
--
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
Re: [PR] HDDS-9405. Make PipelineStateMap thread-safe [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5410:
URL: https://github.com/apache/ozone/pull/5410#discussion_r1352101488
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java:
##########
@@ -272,162 +178,145 @@ List<Pipeline> getPipelines(ReplicationConfig replicationConfig,
Preconditions
.checkNotNull(excludePipelines, "Pipeline exclude list cannot be null");
- List<Pipeline> pipelines = null;
- if (state == PipelineState.OPEN) {
- pipelines = new ArrayList<>(query2OpenPipelines.getOrDefault(
- replicationConfig, Collections.emptyList()));
- if (excludeDns.isEmpty() && excludePipelines.isEmpty()) {
- return pipelines;
- }
- } else {
- pipelines = new ArrayList<>(pipelineMap.values());
- }
-
- Iterator<Pipeline> iter = pipelines.iterator();
- while (iter.hasNext()) {
- Pipeline pipeline = iter.next();
- if (!pipeline.getReplicationConfig().equals(replicationConfig) ||
- pipeline.getPipelineState() != state ||
- excludePipelines.contains(pipeline.getId())) {
- iter.remove();
- } else {
- for (DatanodeDetails dn : pipeline.getNodes()) {
- if (excludeDns.contains(dn)) {
- iter.remove();
- break;
- }
- }
- }
- }
-
- return pipelines;
+ return getPipelines(pipeline -> pipeline.getPipelineState() == state &&
Review Comment:
I can see a problem here, as the old code handled open pipelines in a special case, where all the open pipelines for a given replication config are stored in a list, and that list can be returned directly.
The new code must iterate all pipelines to find a list of those to return so it will be much slower than the original.
The reason open pipelines are managed this way, is because that is a common request from other parts of the system. Either to get the list of open pipelines or to get the count.
For example the count of open EC-6-3 pipelines can be answered with a simple map.get().size() call, with no iteration or allocation of any objects, but this change would break that fast lookup.
--
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
Re: [PR] HDDS-9405. Make PipelineStateMap thread-safe [ozone]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1755746647
@sodonnel Yes, the binaries were build off the recent master.
The same test against the Ratis pipelines showed similar results.
The issue we see is related neither to the number of pipelines nor to their kind, rather to the number of clients (requests) that trigger the Client->OM->SCM chain. More clients/requests results in more contention on the SCM lock.
During the test we uploaded 20MB files using more than 500 clients, so managed to fill up 5GB containers pretty quickly.
Down the line the first request that acquired the lock in the state manager makes a request to Ratis at `org.apache.hadoop.hdds.scm.ha.SCMRatisServerImpl.submitRequest` to negotiate the pipeline state update with other SCMs.
So what I see is that the bottleneck here is not the ratis itself (deserves a separate task), but the code above it that does not allow more than one ratis request at a time.
I'm strongly convinced we need some kind of lock striping here to make SCM scalable. To achieve that first we need to make the existing SCM locking more comprehensible and covered with tests, hence this task.
--
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
Re: [PR] HDDS-9405. Make PipelineStateMap thread-safe [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1757933553
The solution to the problem you are seeing, which is stalled container selection on SCM, is likely to be moving pipeline creation into the background, and probably pipeline closing too.
Ratis pipelines tend to not close, as they are long lived, but for EC we are creating pipelines on demand. We probably need to create the EC pipelines in the background too, and then handle closing then out of the hot read path.
Reworking how EC pipelines get created and allocated is a safer and hopefully small change that re-writing much of SCM to introduced stripped locking.
--
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
Re: [PR] HDDS-9405. Make PipelineStateMap thread-safe [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1754961383
I checked the linked Jira, and the problem you have mentioned is around EC pipelines. I am not sure we need to re-write the entire pipeline manager due to this. More, we need to consider how the EC pipelines are allocated, and perhaps close EC pipelines as soon as the container is closed. I had an earlier attempt to do this, but I had to revert it as it caused some problem with locking around Ratis - https://issues.apache.org/jira/browse/HDDS-9151 . That is one problem that is slowing down the EC pipeline selection as it starts iterating the list and potentially finds many pipelines to be closed.
Allocating new pipelines should be a very fast operation, even under a single lock.
Were the performance tests run against a recent apache master version?
--
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
Re: [PR] HDDS-9405: Make PipelineStateMap thread-safe [ozone]
Posted by "ibrusentsev (via GitHub)" <gi...@apache.org>.
ibrusentsev commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1754704243
@sodonnel, I extended description and rollbacked formatting changes where it was possible
--
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
Re: [PR] HDDS-9405. Make PipelineStateMap thread-safe [ozone]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1766497432
I checked the test results again and see that the performance issues were observed on EC only. Hard to say only by the test results that Ratis is not affected since Ratis is several times slower than EC. It would be nice to have at least one replication type to be on par with Hadoop, so I was planning to look closer at the Ratis in a separate task and concentrate on EC here.
Indeed the profiles are showing the call to Ratis to replicate the SCM changes across the SCM. But they also show that the other threads (even if they don't need the result of that replication) are still blocked on the same lock.
If there are ways to speed up the state replication - I would be glad to learn them and/or fix the issues. But regardless of the underlying changes, the state management code seems overcomplicated.
The goal of this task and a few following ones was to make SCM locking a bit easier to understand and test before even starting to think of striped locking.
--
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
Re: [PR] HDDS-9405: Make PipelineStateMap thread-safe [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5410:
URL: https://github.com/apache/ozone/pull/5410#issuecomment-1752788653
In the PR description, can you provide some details of the problems you saw which this re-write solves?
Also, there are many changes which are just formatting changes, eg at the start of methods. Please avoid making format only changes as it makes the PRs more difficult to review and to backport to other branches if needed.
--
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