HDDS-4473. Reduce number of sortDatanodes RPC calls

[adoroszlai]

What changes were proposed in this pull request?

KeyManagerImpl#listStatus and the sortDatanodeInPipeline helper method sort datanodes using individual RPC call for each key location info.

Improvements in this change:

1. Process only the "latest" location version instead of all versions. If I understand correctly, only the "latest" version is used for read.
2. Keep track of processed pipelines. If another key or location refers to the same pipeline, avoid repeating the query.

Possible improvement left for later: send a single sortDatanodes request for all datanodes in all relevant pipelines, then create the per-pipeline lists locally.

https://issues.apache.org/jira/browse/HDDS-4473

How was this patch tested?

Added unit test for sortDatanodes and added check in existing test case for listStatus.

[sodonnel]

I noticed this code before, in that we have a batch call to get the pipelines, but then for a key with multiple blocks, we must make a call per block to sort the datanodes. Am I correct in thinking this is mostly a problem with large keys which have many blocks?

Do you think it would make sense to add the sort flag to the getContainerWithPipeline and getContainerWithPipelineBatch so we can have the pipeline returned sorted with a single RPC call, rather than having to make at least two calls?

[adoroszlai]

Am I correct in thinking this is mostly a problem with large keys which have many blocks?

I think so, and also for listStatus with many entries in the result.

Do you think it would make sense to add the sort flag to the getContainerWithPipeline and getContainerWithPipelineBatch so we can have the pipeline returned sorted with a single RPC call, rather than having to make at least two calls?

Actually that's a great idea, better than the third (and not yet implemented) improvement I proposed, which would still require 2 batch requests.

I think we have to pay attention to compatibility, though. Client would need to check if SCM supports this new flag, so that it can get sorted pipeline from both new and old SCM.

[swagle]

cc: @avijayanhwx for compat check though upgrades.

[adoroszlai]

Filed HDDS-4523 for further improvement suggested by @sodonnel.

[sodonnel]

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.

[adoroszlai]

Thanks @sodonnel for digging the details on pipeline refresh, and pointing out pipeline ID being random. In this case I think we should use the set of node IDs, instead of the pipeline ID, as cache key.

[sodonnel]

we should use the set of node IDs, instead of the pipeline ID, as cache key.

Good idea - I think that will work much better. I checked the latest version and it looks good to me. I just have one small comment you can consider if you think it makes sense.

[sodonnel]

LGTM +1.

[adoroszlai]

Thanks @sodonnel for the reviews.