HDDS-9983. Changed snapshot list API to return continuation token rather than using last element from the previous page's response.

[hemantk-12]

What changes were proposed in this pull request?

Currently, the last element from the current page's response is used as a continuation token for the list snapshot API. Because of that, every time a client has to make an extra call to know if there are more entries. This change is to return the continuation token from the server and the next page will be fetched after the provided token.

What is the link to the Apache JIRA

HDDS-9983

How was this patch tested?

Updated existing unit tests.

[swamirishi]

@hemantk-12 Thanks for the patch. I really like the the idea here, but I have a minor nitpicky comment on the response we are sending here. In my opinion sending a single boolean value should be sufficient to what we are trying to achieve here.

[ayushtkn]

how does this behave in case of new server & old client or viceversa?

[swamirishi]

how does this behave in case of new server & old client or viceversa?

I believe it should be backward compatible with client. Old client will work seemlessly in a sense it won't use the additional response. But newer client will not work with old server. @hemantk-12 we should ensure this, if the lastSnapshotResponse doesn't exist we should rely on the iterator's last key instead falling back to the original logic.

[hemantk-12]

how does this behave in case of new server & old client or vice-versa?

Protobuf change is backward and forward compatible in itself.

I think the only change which can break is forward compatibility (will fetch only one page) of OzoneManagerProtocolClientSideTranslatorPB wrapper as @swamirishi mentioned.

@swamirishi If lastSnapshot doesn't exist in the response, we should rely on the iterator's last key instead falling back to the original logic is not making any difference. I think it will defect the whole purpose of the change.

The better approach would be to set lastSnapshot (here) either when API has it in response or when list size is equal to maxListResult.

    ...
    ...
    String lastSnapshot = null;

    if (response.hasLastSnapshot()) {
      lastSnapshot = response.getLastSnapshot();
    } else if (snapshotInfos.size() == maxListResult) {
      // This is to make sure that the change (HDDS-9983) is forward compatibility.
      // Set lastSnapshot only when current list size is equal to maxListResult
      // and there is possibility of more entries.
      lastSnapshot = snapshotInfos.get(maxListResult - 1).getName();
    }

    return new ListSnapshotResponse(snapshotInfos, lastSnapshot);

I made the change according to this. Let me know your thoughts.

[swamirishi]

LGTM Thanks for the patch @hemantk-12 . From the changes we are reducing the extent of another OM request after the last page but we could be hitting another redundant OM request in the case mentioned in the comments. But anyhow we can go ahead with this change for now.

[swamirishi]

@hemantk-12 Do you want a review from someone else on this? Or else we can merge this.

[hemantk-12]

how does this behave in case of new server & old client or viceversa?

@ayushtkn do you want to give it another look?

[ayushtkn]

@hemantk-12 If you folks are convinced, feel free to merge, thanks for holding

[sumitagrawl]

@hemantk-12 overall code looks good other than existing comments

[hemantk-12]

Thanks for the review @sumitagrawl. I've addressed or resolved most of the comments. Let me know if there is any compatibility break change otherwise, it should be good to merge.

[sumitagrawl]

@hemantk-12 LGTM

[hemantk-12]

Thanks @swamirishi @sumitagrawl and @ayushtkn for the review.