Add paginated payment listing API#959
Conversation
|
👋 Thanks for assigning @joostjager 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.
fbb20fc to
0135e57
Compare
|
Changed to continue using the in memory entires in the |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Seems tests are failing and this also needs another rebase.
0135e57 to
52bdc41
Compare
fixed but seems like we're still getting random ci failures from out of storage |
|
The discussion above mentions that we eventually need to stop keeping all payments in memory. Since #269 only tracks pagination, would it be worth opening a separate issue for the remaining scalability work? |
5a3e7a3 to
607aa48
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. AI-assisted-by: OpenAI Codex
607aa48 to
cf14127
Compare
| 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.
Mixing disk reads and memory reads seems risky. Is there locking that prevents discrepancies between the two?
If we go with in-memory values for now, it might be better to store the pagination information in memory too.
| })?; | ||
|
|
||
| let locked_objects = self.objects.lock().expect("lock"); | ||
| let objects_by_store_key: HashMap<String, &SO> = |
There was a problem hiding this comment.
It looks inefficient to build an all-values map for each page?
| pub fn list_payments(&self) -> Vec<PaymentDetails> { | ||
| self.payment_store.list_filter(|_| true) | ||
| /// Retrieves a page of payments from the underlying paginated store, ordered from most | ||
| /// recently created to least recently created. |
There was a problem hiding this comment.
P2, backend migration loses payment chronology
The migration helper copies keys in arbitrary list_all_keys order. Destination stores assign new creation ordering from that write sequence, so payment order after migration can differ from the documented newest-payment-first order. The migration test hides this by sorting both results by payment ID.
Preserve ordering metadata or migrate entries in an order that reconstructs the original chronology. The test should compare pagination order without sorting.
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.