Skip to content

Conversation

@tanvipenumudy
Copy link
Contributor

@tanvipenumudy tanvipenumudy commented Jul 25, 2023

What changes were proposed in this pull request?

ListKeys currently reports all the blocks for each of the keys. There are multiple use-cases where this is not required:

  1. Shell commands to list keys
  2. S3G list buckets API

The goal is to implement a listKeys API that can be skipped and does not need to include block information. The keys are only listed if the user has appropriate permissions.

What is the link to the Apache JIRA

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

How was this patch tested?

@tanvipenumudy
Copy link
Contributor Author

Could you please take a look @kerneltime, @sadanand48, @duongkame, thanks!

@tanvipenumudy tanvipenumudy marked this pull request as draft July 25, 2023 16:26
@tanvipenumudy
Copy link
Contributor Author

tanvipenumudy commented Jul 25, 2023

There are a few Git CI/CD check failures - taking a look at the same.

Edit: Resolved the Git CI/CD check failures.

@tanvipenumudy tanvipenumudy marked this pull request as ready for review July 26, 2023 19:27
@hemantk-12
Copy link
Contributor

hemantk-12 commented Jul 26, 2023

Just a thought, wouldn't it be easier if you just don't populate extra information when listing keys for the listKeys API? By default API will return only necessary information. An extra parameter can be added to the existing list API to return full key information. Or vice versa for backward compatibility.

I see we have something similar already in listKeys API.

@kerneltime
Copy link
Contributor

Just a thought, wouldn't it be easier if you just don't populate extra information when listing keys for the listKeys API? By default API will return only necessary information. An extra parameter can be added to the existing list API to return full key information. Or vice versa for backward compatibility.

I see we have something similar already in listKeys API.

We discussed this and felt it is simpler to keep separate lightweight APIs that behave in one way rather than APIs that are multi modal and have variable responses based on varying parameters. Keeping the APIs simple will also help in measuring runtime performance of the APIs for OM vs. not knowing the parameters involved.

@kerneltime
Copy link
Contributor

Unfortunately, we still need to deserialize the metadata to complete the request which is an additional load but we should be able to greatly reduce the payload and skip block information altogether.

Copy link
Contributor

@errose28 errose28 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 working on this @tanvipenumudy. Do we have follow up jiras to integrate this with the higher level Ozone interfaces that don't need block information:

  • o3 CLI: (ozone sh key list --light or something like that)
  • S3 ListObject requests
  • ofs -ls

If these are planned could you link the jiras together or group them as subtasks under a common parent?

@hemantk-12
Copy link
Contributor

Just a thought, wouldn't it be easier if you just don't populate extra information when listing keys for the listKeys API? By default API will return only necessary information. An extra parameter can be added to the existing list API to return full key information. Or vice versa for backward compatibility.
I see we have something similar already in listKeys API.

We discussed this and felt it is simpler to keep separate lightweight APIs that behave in one way rather than APIs that are multi modal and have variable responses based on varying parameters. Keeping the APIs simple will also help in measuring runtime performance of the APIs for OM vs. not knowing the parameters involved.

Thanks @kerneltime. Based on your comment, I see why we are going with new API approach.

@tanvipenumudy the motivation behind this change is not very clear from the PR's description or attached jira. If there is a feature jira or design doc where motivation is captured, it would be great if you attached to PR or jira.

@tanvipenumudy
Copy link
Contributor Author

tanvipenumudy commented Jul 28, 2023

Thank you for the reviews @hemantk-12, @kerneltime and @errose28, I will be working on incorporating the changes as suggested.

At the moment, we haven't created any follow-up Jiras for the integration part that was mentioned. However, I will make sure to create a parent Jira to group these tasks together/attach a design document that captures the motivation behind this change.

Edit: Parent Jira HDDS-2328

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants