sodonnel commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-735943927
This change looks good to me and I am +1 to commit it. However I would like
to highlight one issue that may make this change less effective.
In SCMClientProtocolServer this is the code where the refreshPipelinesBatch
call lands.
```
private ContainerWithPipeline getContainerWithPipelineCommon(
long containerID) throws IOException {
final ContainerID cid = ContainerID.valueof(containerID);
final ContainerInfo container = scm.getContainerManager()
.getContainer(cid);
if (safeModePrecheck.isInSafeMode()) {
if (container.isOpen()) {
if (!hasRequiredReplicas(container)) {
throw new SCMException("Open container " + containerID + " doesn't"
+ " have enough replicas to service this operation in "
+ "Safe mode.", ResultCodes.SAFE_MODE_EXCEPTION);
}
}
}
Pipeline pipeline;
try {
pipeline = container.isOpen() ? scm.getPipelineManager()
.getPipeline(container.getPipelineID()) : null;
} catch (PipelineNotFoundException ex) {
// The pipeline is destroyed.
pipeline = null;
}
if (pipeline == null) {
pipeline = scm.getPipelineManager().createPipeline(
HddsProtos.ReplicationType.STAND_ALONE,
container.getReplicationFactor(),
scm.getContainerManager()
.getContainerReplicas(cid).stream()
.map(ContainerReplica::getDatanodeDetails)
.collect(Collectors.toList()));
}
return new ContainerWithPipeline(container, pipeline);
}
```
For open containers the current pipeline is returned, but for a closed
container, a new "pipeline" is created. This is not a real ratis pipeline - its
just a list of nodes. If you follow the logic to SimplePipelineProvider, the
pipeline ID is a random ID:
```
public Pipeline create(ReplicationFactor factor,
List<DatanodeDetails> nodes) {
return Pipeline.newBuilder()
.setId(PipelineID.randomId())
.setState(PipelineState.OPEN)
.setType(ReplicationType.STAND_ALONE)
.setFactor(factor)
.setNodes(nodes)
.build();
}
```
This means, that even if several blocks have the same set of DNs, they will
still be distinct pipelines, and hence the caching will not work. The caching
will only be effective for open containers.
There are two places this logic is used - listStatus and lookupkey /
getOzoneFileStatus.
I guess the place where this could be a win is in listStatus, but if most
keys are part of closed containers (which would be the usual case) the sorted
pipeline cache introduced here may cause more overhead than it saves.
I also wonder - why does ListStatus need to return the pipeline and
optionally sort the pipeline for all keys, all the time?
I guess we can have two uses - simply listing the directory and displaying
the contents - usually locations is not needed here. Second, an application
list the folders with the intention of reading each key - in this second case
the locations are 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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
