Add paginated payment listing API#959
Conversation
Add `DataStore::list_page`, returning one page of stored objects in reverse creation order together with a token to fetch the next page. Reads within a page are spawned concurrently, and keys removed between listing and reading are skipped rather than failing the page. Expose this via a new `Node::list_payments_paginated` method along with bindings support for `PageToken` and `PaymentDetailsPage`, allowing users to retrieve payments page-by-page instead of copying the entire payment history across the FFI boundary at once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Few questions on the general approach / general direction going forward.
| }, | ||
| }); | ||
|
|
||
| uniffi::custom_type!(PageToken, String, { |
There was a problem hiding this comment.
Can we use the remote type rather than adding a custom type (as we try to get rid of the latter over time)?
| /// Pass `None` to start listing from the most recently created payment. If the returned | ||
| /// [`PaymentDetailsPage::next_page_token`] is `Some`, pass it to a subsequent call to | ||
| /// retrieve the next page. | ||
| pub fn list_payments_paginated( |
There was a problem hiding this comment.
Hmm, do we really want an additional method? At some point (hopefully for v0.9) we'll stop keeping all entries in memory and at that point at the latest the current shape of list_payments isn't feasible anymore. So should this just replace list_payments? Or not yet?
| pub(crate) async fn list_page( | ||
| &self, page_token: Option<PageToken>, | ||
| ) -> Result<(Vec<SO>, Option<PageToken>), Error> { | ||
| let response = PaginatedKVStore::list_paginated( |
There was a problem hiding this comment.
Currently, we're still leaning on all DataStore entries being kept in memory everywhere else (which we should change soon as mentioned above). So right now it's a bit odd to have the paginated version being the only one calling through to KVStore, and not even using any cache (so it's likely pretty slow).
So, should this just use the in-memory entries? If not, we could consider already making the jump to not keep all entries in memory, but then we'd need some (presumably LRU?) caching logic for DataStore first, before we add pagination support/switch it to use pagination?
| let mut spawn_iter = response.keys.iter(); | ||
| let mut read_handles = VecDeque::with_capacity(DATA_STORE_READ_CONCURRENCY_LIMIT); | ||
| for key in spawn_iter.by_ref().take(DATA_STORE_READ_CONCURRENCY_LIMIT) { | ||
| read_handles.push_back(tokio::spawn(KVStore::read( |
There was a problem hiding this comment.
Drive-by question: are there plans for something like list_paginated_values, so SQL backends can fetch a page of serialized values in a single query instead of doing list_paginated plus one read per key?
There was a problem hiding this comment.
Seems like a follow-up optimization that could eventually make sense (though, maybe even worth benchmarking if necessary for the concrete use cases). Mind opening an issue for it so we don't forget to revisit?
There was a problem hiding this comment.
4 participants
Add
DataStore::list_page, returning one page of stored objects in reverse creation order together with a token to fetch the next page. Reads within a page are spawned concurrently, and keys removed between listing and reading are skipped rather than failing the page.Expose this via a new
Node::list_payments_paginatedmethod along with bindings support forPageTokenandPaymentDetailsPage, allowing users to retrieve payments page-by-page instead of copying the entire payment history across the FFI boundary at once.