-
Notifications
You must be signed in to change notification settings - Fork 132
chore: Improved StateView and StateSnapshot lifetime logging #2452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from all commits
d20d632
73472c2
2832c42
39f0d4f
64701c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this is where we would want to do this because there is a lack of actionable information. What do we now do once we receive this? We need to identify which query this is, but we have no way of doing so.. Perhaps we could explore a timer within the actual snapshot itself, and each snapshot taken automatically gets the caller LoC information embedded?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added a LoC WARN log on long-held StateViews (StateSnapshots are not instantiated per-query, StateViews are). The per-block log is still important in case we ever get snapshots or views that never end.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those two things are essentially synonyms. We should try improve our naming here. I assume its a snapshot because rocksdb calls them snapshots? Perhaps Is there a downside to having just a single one, instead of separate types? I can't imagine a snapshot is expensive to hold temporarily.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. StateViews and StateSnapshots are not synonyms - any number of views (one per API request) can map to a single snapshot. The view is there to enforce the invariants / API appropriate for accessing snapshot + SQL data consistently (block scoped requests). The snapshot is the non-SQL data itself.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that's what they are in our code. I'm saying the word If I say
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. However, I find the terms view and snapshot to be quite appropriate here (differentiating between the data itself [snapshot] and how it is accessed [view]). Snapshot (computing):
View (database):
Alternatives you might prefer, LMK:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting here so we can thread on this discussion. File isn't relevant.
Could you provide an overview of how things work wrt state views and snapshots, and pruning history. I've lost a lot of context here.
What I was hoping we would have is something like the following.
We have a
ReadOnlyStatetype struct. Holders (i.e. gRPC requests) use this to obtain a singlularStateView. This view comprises a RocksDb snapshot, a SQLite transaction, and the proof + chain tips (as numbers, not channels).The data provided within the view is internally consistent i.e. it represents the complete state at a specific block height (chain tip). This
StateViewimplements the query methods aka it becomes our "state transaction" type.In order to guarantee the above, we cannot prune data out from under it while that object is being constructed. So we have to be careful about deleting or invalidating old references to it.
When we delete old "views", we have to first check that no-one is still holding it. If they are, then we can't delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer to that is basically in
PublishedGeneration::advance().The writer keeps one
Weak<StateSnapshot>per published generation (PublishedGenerations::record,view/snapshot.rs:100-106). While applying a block — before the DB commit, so the result can feed pruning inside the same transaction — it callsadvance(chain_tip)(view/snapshot.rs:118-130, invoked atwriter/worker.rs:230), which:Arcis gone (strong_count() == 0) — nobody holds them, safe to have pruned,Weakis still alive — that'soldest_pinned,prune_tip), capped atSNAPSHOT_PRUNE_LAG_CAPblocks of lag so one leaked reader can't stall pruning forever (accepting a historical-read race for that reader past the cap — logged loudly, see below).The resulting
prune_tipis passed intodb.apply_block(..), and history pruning runs inside that same DB transaction (writer/worker.rs:245-256).So the "can't delete out from under a holder" guarantee is that pruning simply doesn't advance past a live generation until that generation's
Arcrefcount hits zero (lastStateView/StateSnapshotreference dropped).What we have today is aligned with this except that we still have the channel based proof and chain tips in
Staterather thanStateView(reminder: you get a view viaState::view()orState::with_view()). Those channels are used for long-lived subscriptions and so do not make sense to come through theStateView. We cannot tie the views' lifetimes to a long-lived subscription for obvious reasons.