Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added get all keys feature #65

Closed
wants to merge 9 commits into from

Conversation

danielbatica
Copy link

I added a new API to the store: getAllKeys. It returns the exiting keys for the partition.
Added impl, unit tests, e2e, docs.

Motivation and Context

We use aio-state-lib for Adobe's AEM Screens Cloud. We need to get all stored keys.
Jira story: http://jira.corp.adobe.com/browse/SCRNS-1287

How Has This Been Tested?

Added e2e test that passes. Used an AIO runtime namespace

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@danielbatica danielbatica changed the title Get all feature feat: added get all keys feature Apr 24, 2021
@meryllblanchet
Copy link
Contributor

Thanks for this complete PR, @danielbatica ! We'll review next week, stay tuned.

@bdelacretaz
Copy link

I'm not working on this code but I have alarms go on when I see a getAllSomething method that doesn't set limits on the number of items returned ;-)

What if you have a hundred million keys?

I think the method should either implement some form of paging for the results, or at least set a limit on the size of the array returned.

@danielbatica
Copy link
Author

danielbatica commented Apr 26, 2021

I'm not working on this code but I have alarms go on when I see a getAllSomething method that doesn't set limits on the number of items returned ;-)

What if you have a hundred million keys?

I think the method should either implement some form of paging for the results, or at least set a limit on the size of the array returned.

Makes sense, It would be easy if the underlying cosmosdb would provide that since this is just a wrapper. I think this might help https://docs.microsoft.com/en-us/javascript/api/@azure/cosmos/feedoptions?view=azure-node-latest#_azure_cosmos_FeedOptions_continuationToken

@meryllblanchet
Copy link
Contributor

Hi @danielbatica , we've been discussing your PR and we'll keep it on hold for now: we have indeed a very similar PR from @sandeep-paliwal which had been deferred, for the following reasons:

  • We want to be cautious about what and how we expose in terms of security, runtime performance and costs. For the two last items, this kind of operation run against a large set of States can be very impactful for all users, as we do share a single storage and RUs for all underlying namespaces.

  • We consider an abstracted search with pagination as an alternative, with restrictions on what can be searched and how

  • We want the abstraction to be provider agnostic to preserve the Bring Your Own Storage paradigm possible (e.g. and let developers bring a DynamoDB instance or other if needed)

Nevertheless this feature is frequently asked by our users. As an immediate next step, @sandeep-paliwal will go through a usage analysis of our OOTB storage so that we could assess the possible impact as early mentioned.
We'll keep you in the loop and follow-up with you to enhance your PR according to our feedback.

cc @sarahxxu for her additional input

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #65 (4c45cb3) into master (47a0566) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          173       182    +9     
  Branches        36        36           
=========================================
+ Hits           173       182    +9     
Files Changed Coverage Δ
lib/StateStore.js 100.00% <100.00%> (ø)
lib/impl/CosmosStateStore.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shazron shazron added the deferred Issues and pull requests that require more thought, or add cost/complexity that must be evaluated. label Oct 13, 2021
@purplecabbage purplecabbage removed their request for review April 14, 2022 02:25
@shazron shazron mentioned this pull request Apr 27, 2022
10 tasks
@shazron shazron changed the base branch from master to 3.x February 22, 2024 00:22
@moritzraho
Copy link
Member

moritzraho commented Aug 12, 2024

closing as this is implemented in the new State majors (> v4). Legacy State users can refer to this PR to manually implement listing.

@moritzraho moritzraho closed this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred Issues and pull requests that require more thought, or add cost/complexity that must be evaluated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants