Skip to content

Conversation

@ArafatKhan2198
Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 commented May 17, 2023

What changes were proposed in this pull request?

The getContainers() method in the container Endpoint has been modified to exclude the container specified by the prevKey parameter from the list of containers in the ContainerResponse.

The getContainers() method changes ensure that:

  1. Skipping the container with the same ID as prevKey: The container stream is filtered to exclude the container matching prevKey, ensuring it is not included in the results.

  2. To increase the prevKey by 1: We simply add 1 to the current value of prevKey. This is because the container IDs are arranged in ascending order. By doing this, we remove the first container from the list, which corresponds to the container with a Container ID equal to prevKey. As a result, this prevents that particular container from appearing in the API response.

  3. Providing the last container ID in container API Response: The last container ID is determined based on the fetched container metadata. If no containers were fetched, last container ID is set to prevKey. Otherwise, it is obtained from the ID of the last container in the list. This information enables pagination and using the last container ID as the new prevKey value for subsequent requests.

What is the link to the Apache JIRA

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

How was this patch tested?

  • Modified the Existing Unit Tests to test out the imrpovements
  • Manual Testing :-
// JSON response with prevKey=2 

{
  "data": {
    "totalCount": 4,
    "prevKey": 6,
    "containers": [
      {
        "ContainerID": 3,
        "NumberOfKeys": 27,
        "pipelines": null
      },
      {
        "ContainerID": 4,
        "NumberOfKeys": 29,
        "pipelines": null
      },
      {
        "ContainerID": 5,
        "NumberOfKeys": 26,
        "pipelines": null
      },
      {
        "ContainerID": 6,
        "NumberOfKeys": 28,
        "pipelines": null
      },
    ]
  }
}

@ArafatKhan2198
Copy link
Contributor Author

@devmadhuu @dombizita @krishnaasawa1 Can you please take a look.

@devmadhuu
Copy link
Contributor

@ArafatKhan2198 thanks for working on this patch, can you also pls add a test case with adding non-sequential container Ids and do all assertions ?

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ArafatKhan2198, Thanks for working on this. Please find few comments.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the patch, LGTM.

@devmadhuu
Copy link
Contributor

Thanks for updating the patch @ArafatKhan2198 . LGTM +1

@ArafatKhan2198 ArafatKhan2198 requested a review from dombizita May 22, 2023 18:41
@dombizita dombizita merged commit a10d31c into apache:master May 23, 2023
@dombizita
Copy link
Contributor

thanks for the patch @ArafatKhan2198! thanks for the review @ashishkumar50 @devmadhuu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants