You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "adoroszlai (via GitHub)" <gi...@apache.org> on 2023/05/23 10:08:37 UTC
[GitHub] [ozone] adoroszlai opened a new pull request, #4758: HDDS-8674. Allow more EC pipelines based on number of volumes
adoroszlai opened a new pull request, #4758:
URL: https://github.com/apache/ozone/pull/4758
## What changes were proposed in this pull request?
Currently EC pipelines are limited to 5 per EC scheme, which does not scale with the size of the cluster.
As a simple first step for scaling EC pipeline/container count, `WritableECContainerProvider` should dynamically adjust the limit based on the number of healthy data volumes available in the cluster.
To retain behavior in smaller clusters, the existing `pipeline.minimum` config is used if it is greater than the calculated value.
https://issues.apache.org/jira/browse/HDDS-8674
## How was this patch tested?
Added test case in `TestWritableECContainerProvider`.
https://github.com/adoroszlai/hadoop-ozone/actions/runs/5054873935
--
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 #4758: HDDS-8674. Allow more EC pipelines based on number of volumes
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #4758:
URL: https://github.com/apache/ozone/pull/4758#issuecomment-1559600613
This change looks good, and is surprisingly simple to implement.
There are two things that concern me in the provider code, which are both problems which already exist and are not really part of this PR.
Now that we have greatly increased the pipeline limit, the code that allows a client to pass in excluded containers / nodes worries me a little. If will allow for more and more pipeline to be created. Say, for example, a client node is bad, and it keeps on failing to write to every pipeline it is given for some reason. It really the client node which has the issue, rather than the datanodes. It could keep on requesting more and more pipelines, excluding nodes / containers to and start to grow the pipeline count way beyond the limit. This makes me wonder if we need another limit to prevent the pipelines growing without any bound in this sort of case? Happy for this to be another Jira, or discuss further on it.
This piece of code, where it filters out pipelines without enough space:
```
synchronized (pipeline.getId()) {
try {
ContainerInfo containerInfo = getContainerFromPipeline(pipeline);
if (containerInfo == null
|| !containerHasSpace(containerInfo, size)) {
// This is O(n), which isn't great if there are a lot of pipelines
// and we keep finding pipelines without enough space.
existingPipelines.remove(pipeline);
pipelineManager.closePipeline(pipeline, true);
} else {
if (containerIsExcluded(containerInfo, excludeList)) {
existingPipelines.remove(pipeline);
} else {
return containerInfo;
}
}
} catch (PipelineNotFoundException | ContainerNotFoundException e) {
LOG.warn("Pipeline or container not found when selecting a writable "
+ "container", e);
existingPipelines.remove(pipeline);
pipelineManager.closePipeline(pipeline, true);
}
}
```
There is a comment there about the `existingPipelines.remove()` being O(n), so this could get expensive, so we may need to refactor it a little in another Jira too.
--
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 merged pull request #4758: HDDS-8674. Allow more EC pipelines based on number of volumes
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel merged PR #4758:
URL: https://github.com/apache/ozone/pull/4758
--
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 #4758: HDDS-8674. Allow more EC pipelines based on number of volumes
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4758:
URL: https://github.com/apache/ozone/pull/4758#discussion_r1202532912
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -159,6 +160,16 @@ public ContainerInfo getContainer(final long size,
}
}
+ private int getMinimumPipelines(ECReplicationConfig repConfig) {
Review Comment:
At this code path with be hit for every single block request, I wonder if we should perform this calculation every time? It has to iterate over all nodes in the cluster checking their state and then summing up their disk count. Its probably not needed to recalculate this every single time - we could either calculate it 1 in X times, or have some sort of time based recalculation. Its also called under a synchronized block, so it could become a point of contention in the block allocation flow.
--
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 #4758: HDDS-8674. Allow more EC pipelines based on number of volumes
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4758:
URL: https://github.com/apache/ozone/pull/4758#issuecomment-1560848529
Thanks @sodonnel for reviewing and merging this.
--
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 #4758: HDDS-8674. Allow more EC pipelines based on number of volumes
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4758:
URL: https://github.com/apache/ozone/pull/4758#discussion_r1202569024
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/WritableECContainerProvider.java:
##########
@@ -159,6 +160,16 @@ public ContainerInfo getContainer(final long size,
}
}
+ private int getMinimumPipelines(ECReplicationConfig repConfig) {
Review Comment:
I was thinking we can improve it later by calculating it on certain events, e.g. node report and node state change.
Thanks for spotting the synchronized block, I moved the calculation outside of 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