Introduce generalized DataStore
#544
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
| self.persist(&object)?; | ||
| let updated = locked_objects.insert(object.id(), object).is_some(); |
There was a problem hiding this comment.
Does the order matter here? The opposite order is used in insert_or_update, so there we could have the object in memory but fail persisting.
There was a problem hiding this comment.
Hmm, this one used to be the other way around, but I changed it here (actually should be the only behavioral diff I snuck in, IIRC), exactly because it would be preferable to not update the in-memory version if persistence failed. However, for insert_or_update/update we won't know whether the update is necessary at all until StorableObject::update returns, which is why we unfortunately can't first persist.
There was a problem hiding this comment.
For insert_or_update/update, it seems we log an error but continue. I suppose this predates the PR, but I wonder if we should do something else like have a queue of ids that still need persistence. Though if we eventually crash that information would be loss. Maybe this is more of a storage implementation concern.
There was a problem hiding this comment.
Yeah, we could do that eventually, although it probably would somehow need to fit into a "halt everything on (remote) persistence failure, and resume/restart once we reestablish connectivity" scheme.
tnull
force-pushed
the
2025-05-generalized-data-store
branch
from
f29bdd3 to
6fa3502
Compare
| self.persist(&object)?; | ||
| let updated = locked_objects.insert(object.id(), object).is_some(); |
There was a problem hiding this comment.
For insert_or_update/update, it seems we log an error but continue. I suppose this predates the PR, but I wonder if we should do something else like have a queue of ids that still need persistence. Though if we eventually crash that information would be loss. Maybe this is more of a storage implementation concern.
tnull
force-pushed
the
2025-05-generalized-data-store
branch
from
6fa3502 to
0561bc4
Compare
Squashed without further changes. |
We increasingly feature different data stores that essentially do the same thing, mod different data types. Here we add a generalized `DataStore` that will be used to DRY up our logic.
.. we utilize the just-introduced generalized `DataStore` for our `PaymentStore`.
tnull
force-pushed
the
2025-05-generalized-data-store
branch
from
0561bc4 to
75aa069
Compare
Should be, I re-ran CI, will merge if it passes. |
We already have a 'store' pattern in a few places in our codebase (peer store, payment store), and for #425 we're about to add yet another one. To at least somewhat DRY up the storage logic, we here introduce a generalized
DataStoreand migrate ourPaymentStoreto it. This is a prefactor to #425.(cc @moisesPompilio)