-
Notifications
You must be signed in to change notification settings - Fork 137
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
State Storage - State #571
Conversation
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.
Looks great overall!
I think the implementation details can go into the reference implemention
section. Can you add a more high level overview in specification and move the rest? Can you also mention what the naive / brute force approach would be and why it is not feasible?
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.
Once we have a good design for the integration with cold storage let's also add a short description of how that will work.
neps/nep-0568.md
Outdated
Mappings in `ShardUIdMapping` persist as long as any descendant shard still references the ancestor shard’s data. | ||
When no descendants rely on a particular ancestor shard, its mapping entry can be removed, allowing the ancestor’s data to be garbage collected. | ||
If a full state sync is performed for a child shard (downloading and storing all its data independently), | ||
the mapping to the ancestor shard can also be removed, as the child’s data is now directly stored under its own `ShardUId`. | ||
|
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 cleanup shouldn't depend on state sync. I think we should cleanup the mapping when the node stops tracking all children / descendant shards. Also can you mention that the mapping will be permanent on archival nodes?
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.
looks great, thanks!
Once merged into the main PR, please check if there any any lints that need fixing and address those - feel free to commit directly, no need for another PR. |
No description provided.