Add paginated payment listing API#959
Conversation
|
👋 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.
| /// 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?
There was a problem hiding this comment.
Yeah didn't originally because of the second point but changed here to now replace the normal list_payments
| 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?
There was a problem hiding this comment.
Changed to just use in memory entries. Changing to not keep everything in memory would be a separate PR and maybe should wait for 0.9 as we have a lot of in flight storage changes right now.
| 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.
benthecarman
force-pushed
the
paginated-payments
branch
from
f147b70 to
fbb20fc
Compare
Replace the full payment listing API with paginated listing so callers can migrate away from fetching every payment at once. Deprecate the filtering helper because it still implies scanning the full in-memory store.
benthecarman
force-pushed
the
paginated-payments
branch
from
fbb20fc to
0135e57
Compare
|
Changed to continue using the in memory entires in the |
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.