Support tiered data storage#692
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
4 times, most recently
from
29f47f3 to
264aa7f
Compare
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
264aa7f to
493dd9a
Compare
|
🔔 14th Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
2 times, most recently
from
a30cbfb to
1e7bdbc
Compare
|
🔔 15th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 16th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Thanks for looking into this and excuse the delay here!
I did a first pass, generally this looks already pretty good, but it is a huge PR and in some areas could be simplified. For instance, we should drop the generic Retry logic as the concrete implementations would already implement that if they need it. Likewise, we shouldn't fallback to the backup for now as it's really only meant as disaster recovery (KVStore caching is out-of-scope for this PR now, even though we might want to explore that soon, too). There is also no need to replicate the write-ordering locks in TierStore, but we'll need them in ForeignKVStoreAdapter (which might be mergeable with DynStore?).
Generally, if you find opportunities to reduce the size of the changeset here it would be appreciated. It would also be cool if you could try to break up the PR into more feature commits, but feel free to leave as is if not.
| } | ||
| } | ||
|
|
||
| #[async_trait] |
There was a problem hiding this comment.
Probably not worth taking a dependency just to desugar impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.
There was a problem hiding this comment.
Probably not worth taking a dependency just to desugar
impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.
Well, it's 'the official'/supported way to do async traits with Uniffi: https://mozilla.github.io/uniffi-rs/latest/futures.html#exporting-async-trait-methods
There was a problem hiding this comment.
Does uniffi require that in some way? Its just a comically-overkill way to sugar impl Future which is kinda nuts...
There was a problem hiding this comment.
Oh, worse, it looks like async_trait desugars to the Pin<Box<dyn ...>> version...which is dumb but at least for uniffi it shouldn't matter cause we'd have to do that anyway.
There was a problem hiding this comment.
Unfortunately this will need a substantial rebase now that #696 landed, sorry for that!
Besides some tedious rebase work, we now have DynStoreWrapper which can be used on the ffi and the non-ffi side. I do wonder if we can actually just merge that wrapper with the TierStore, as well as the ForeignKVStoreAdapter/DynStore. Seems all these wrapper structs may not be needed and we could just get away with one trait and one wrapper struct, mostly?
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
2 times, most recently
from
4e221ed to
cb7b044
Compare
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
cb7b044 to
6d8fa02
Compare
|
🔔 1st Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
cadf81f to
1f2cd4d
Compare
|
🔔 2nd Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Feel free to squash fixups!
Unfortunately, cargo test --features uniffi currently fails to compile currently. I think we might also need to revert to implement the same key-level versioning pattern as we have in other stores afterall. Excuse the back-and-forth.
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
1f2cd4d to
d424891
Compare
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
Hi @tnull, I've addressed the most recent feedback in new fix-up commits. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks! Feel free to squash fixups into the respective commit and drop the DROPME commit.
| log_error!(logger, "Failed to setup backup SQLite store: {}", e); | ||
| BuildError::KVStoreSetupFailed | ||
| })?; | ||
| let backup_store: Arc<DynStore> = Arc::new(DynStoreWrapper(backup_store)); |
There was a problem hiding this comment.
Do we need to detect whether the backup store is empty and use MigratableKVStore to backfill the backup store before we continue operating if this is enabled on a pre-existing node? If it's disabled and re-enabled at a later date, how could we detect that the backup is stale and needs to be resilvered?
We probably need to find solutions here, but maybe they should also happen in a follow-up PR, hopefully still landing for the next release?
There was a problem hiding this comment.
These are good questions I didn't consider but I will in the follow up to this PR.
There was a problem hiding this comment.
Can you open an issue for backfilling so we don't forget to look into it?
|
🔔 5th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
2 times, most recently
from
3eef01d to
c4aa2b0
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Unfortunately this needs yet another rebase by now.
|
🔔 6th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
This commit adds `TierStore`, a tiered `KVStore` implementation that routes node persistence across three storage roles: - a primary store for durable, authoritative data - an optional backup store for a second durable copy of primary-backed data - an optional ephemeral store for rebuildable cached data such as the network graph and scorer TierStore routes ephemeral cache data to the ephemeral store when configured, while durable data remains primary+backup. Reads and lists do not consult the backup store during normal operation. For primary+backup writes and removals, this implementation treats the backup store as part of the persistence success path rather than as a best-effort background mirror. Earlier designs used asynchronous backup queueing to avoid blocking the primary path, but that weakens the durability contract by allowing primary success to be reported before backup persistence has completed. TierStore now issues primary and backup operations together and only returns success once both complete. This gives callers a clearer persistence guarantee when a backup store is configured: acknowledged primary+backup mutations have been attempted against both durable stores. The tradeoff is that dual-store operations are not atomic across stores, so an error may still be returned after one store has already been updated. Additionally, adds unit coverage for the current contract, including: - basic read/write/remove/list persistence - routing of ephemeral data away from the primary store - backup participation in the foreground success path for writes and removals
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
c4aa2b0 to
0a240d4
Compare
|
Hi @tnull |
Add native builder support for configuring ephemeral storage and a local SQLite backup mirror. Wrap the primary store in TierStore during node construction and create configured secondary stores using dedicated SQLite database files. Implement paginated listing through TierStore and update filesystem-backed tests to use FilesystemStoreV2. Add full-cycle integration coverage verifying durable backup mirroring.
Preserve call-time ordering for ephemeral writes and removes by routing them through the same versioned lock path as primary-backed mutations. Add regression coverage for stale ephemeral writes and removes.
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
0a240d4 to
10b9721
Compare
| "list", | ||
| )?; | ||
|
|
||
| match (primary_namespace.as_str(), secondary_namespace.as_str()) { |
There was a problem hiding this comment.
Codex:
- [P2] Merge root namespace listings across tiers — /home/tnull/workspace/ldk-node/src/io/tier_store.rs:497-503
With an ephemeral store configured, this namespace-only match catches the empty root namespace because LDK's network graph/scorer namespaces are ("", ""). Durable root keys written through this same TierStore to the primary tier, such as manager, events, peers, node_metrics, and
output_sweeper, then cannot be discovered by list("", ""), even though direct reads still succeed; the duplicated paginated branch has the same problem.
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
... and unfortunately needs yet another minor rebase (sorry, a bunch of PRs are going in right now).
Otherwise we should be close to landing, mod the open comment above.
4 participants
What this PR does
In this PR we introduce
TierStore, a three-tiered (KVStore+KVStoreSync) implementation that manages data across three distinct storage layers based on criticality.Background
As we have moved towards supporting remote storage with
VssStore, we need to recognize that not all data has the same storage requirements. Currently, all data goes to a single store which creates some problems:This PR proposes tiered storage that provides granular control over where different data types are stored. The tiers include:
Additionally, we also permit the configuration of
Nodewith tiered storage allowing callers to:Nodewith a primary store.These configuration options also extend to our foreign interface, allowing bindings target to build the
Nodewith their own (KVStore+KVStoreSync) implementations. A sample Python implementation is provided and tested.Concerns
VssStorehas built-in retry logic. Wrapping it inTierStorecreates nested retries.KVStoreto the FFIRelated Issues