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

Support paginated querying of storage records #3001

Open
5 of 6 tasks
ff137 opened this issue May 31, 2024 · 6 comments
Open
5 of 6 tasks

Support paginated querying of storage records #3001

ff137 opened this issue May 31, 2024 · 6 comments

Comments

@ff137
Copy link
Contributor

ff137 commented May 31, 2024

In a multi-tenant environment with potentially millions of wallets, pagination becomes a critical feature.

Currently, the list wallets endpoint is configured to fetch all. It takes a good few minutes to execute when there are 100s of 1'000s of tenants.

There are probably a lot of endpoints that can benefit from pagination, but off the top of my head, these ones stand out:

Additional features:

I've started working on adding pagination support here: #3000, and so far I've managed to get it working for listing wallets 🎉.

For our use case we need support for the above listed bullet points, so I'll try get those all working for an initial implementation.

Any comments, recommendations or further discussion is welcome!


Related issues: #1919, #2373

@ff137
Copy link
Contributor Author

ff137 commented Jun 13, 2024

@jamshale it's still in progress (I can't reopen it) -- still need to make a PR for connection / cred ex / presentation records. And adding support for ordering in askar

@jamshale jamshale reopened this Jun 13, 2024
@ff137
Copy link
Contributor Author

ff137 commented Jun 19, 2024

Potential endpoints that may need pagination as well:

  • listing created rev regs, in the revocation API
  • listing transaction records, in the endorse_transaction API
  • listing mediation requests, in coordinate_mediation

Those are the only other ones I can see, with potentially large responses

@ff137
Copy link
Contributor Author

ff137 commented Jul 3, 2024

I've discovered a bug with pagination + additional post-filter query parameters.

e.g. for fetching connection records, specifying limit=1 and alias="some alias", then it will return 0 records, despite that alias appearing in some records (i.e. it'll return relevant records when limit is set higher). Post-filtering is being done after the limited fetch, which messes up expected output

@ff137
Copy link
Contributor Author

ff137 commented Jul 3, 2024

This impacts:

  • fetching v1 or v2 credential or presentation exchange records, with query params: connection_id, role, state
  • fetching connection records with: alias, state, their_role, connection_protocol

These are all the post-filter query params for the routes that now have paginated query support. So, limit + offset will not work as expected when using these query params

A workaround is to ignore limit/offset when these query params exist, and to only apply limit/offset after the post filtering ... this would resolve the expected behavior, but there will still be a performance/scalability cost when there are potentially 1000s of records.

@jamshale Do you think this is a reasonable workaround to implement? Behavior <= 0.12.1 was anyway fetching all records. So, while it's not ideal to apply limit/offset after post-filtering, it's still better than having no pagination support ...

@jamshale
Copy link
Contributor

jamshale commented Jul 3, 2024

@ff137 Yes, I think that's the correct way forward for now. I've never understood why these post filters have to be applied the way they do but I think that's good enough as is and we can try and address it in the future.

@ff137
Copy link
Contributor Author

ff137 commented Jul 3, 2024

Agreed. It's of course highly inefficient -- loading each record result as json and post filtering on fields (edit: I see the json deserialization is necessary either way, nvm). I'll do the workaround for now, but will also take a look at askar library for how this may be improved

@jamshale jamshale added the 1.0.0 To be addressed for the ACA-Py 1.0.0 release label Jul 3, 2024
@jamshale jamshale removed the 1.0.0 To be addressed for the ACA-Py 1.0.0 release label Jul 8, 2024
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

No branches or pull requests

2 participants