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: [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]
