HDDS-4448. Duplicate refreshPipeline in listStatus

[adoroszlai]

What changes were proposed in this pull request?

Currently KeyManagerImpl#listStatus issues duplicate refreshPipeline for each file. HDDS-3824 moved refreshPipeline outside the bucket lock. But HDDS-3658 added it back, while keeping the one outside the lock, probably as a result of merge conflict resolution.

This PR removes the refreshPipeline call which is made while holding the bucket lock. It also converts the remaining one to batched (single call with list, instead of N times with single OM key info).

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

How was this patch tested?

Added unit test to verify that a single batched refreshPipeline call is made from listStatus.

https://github.com/adoroszlai/hadoop-ozone/runs/1381172579

[linyiqun]

Good catch, +1.
@adoroszlai , I notice one thing that if we can completely remove field OmKeyArgs#refreshPipeline after HDDS-3658 was merged.

In some place, this option doesn't really make sense now.
Like KeyManagerImpl#getOzoneFileStatus:

  private OzoneFileStatus getOzoneFileStatus(String volumeName,
                                             String bucketName,
                                             String keyName,
                                             boolean refreshPipeline,
                                             boolean sortDatanodes,
                                             String clientAddress)
      throws IOException {
    OmKeyInfo fileKeyInfo = null;
    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
        bucketName);
    ...

      // if the key is a file then do refresh pipeline info in OM by asking SCM
      if (fileKeyInfo != null) {
        // refreshPipeline flag check has been removed as part of
        // https://issues.apache.org/jira/browse/HDDS-3658.
        // Please refer this jira for more details.
        refreshPipeline(fileKeyInfo);
        if (sortDatanodes) {
          sortDatanodeInPipeline(fileKeyInfo, clientAddress);
        }
        return new OzoneFileStatus(fileKeyInfo, scmBlockSize, false);
      }
    }

[bharatviswa504]

Overall LGTM, I have few comments/questions.

[bharatviswa504]

Just a question listStatus is limited by batch size.
But we are making a single Rpc Call to SCM for all the containers we got from this list, is this fine here? (My question is if it has a large number of containerIDs due to larger key sizes, will it have an issue in Rpc Response size.

[adoroszlai]

listKeys() is also limited by batch size and already does the same single refresh call:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

Lines 931 to 933 in d0aa34c

	List<OmKeyInfo> keyList = metadataManager.listKeys(volumeName, bucketName,
	startKey, keyPrefix, maxKeys);
	refreshPipeline(keyList);

[bharatviswa504]

My question is keyList/listStatus is limited by batch size, but if Keys are huge in size, then it might have a long list of container IDS, and then calling SCM with all those containerIDS to fetch ContainerWithPipeline, will it cause an issue of exceeding Rpc response size.

listKeys() is also limited by batch size and already does the same single refresh call:

Then this might be a problem for listKeys also, but do you see this as a problem?

[adoroszlai]

Then this might be a problem for listKeys also, but do you see this as a problem?

I don't see this as an immediate problem.

As far as I understand, batch size for listKeys and listStatus is a convenience for the client, not a safety guarantee for the server. If RPC response size is a problem when performing getContainerWithPipelineBatch call for multiple keys, then the same can be triggered by the client simply by increasing batch size (if there are enough keys in the bucket).

[bharatviswa504]

Ya agreed, not only that thinking more if with normal batch size it is causing issue for containerID's, then it will be same for OMResponse for listKeys also, as it adds KeyInfo and Pipeline.

[bharatviswa504]

+1 LGTM

[adoroszlai]

Thanks @linyiqun for the review, and @bharatviswa504 for review and merge.