-
Notifications
You must be signed in to change notification settings - Fork 655
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
fix(resharding): double resharding state mapping #12688
Conversation
fn set_state_shard_uid_mapping( | ||
&mut self, | ||
split_shard_event: &ReshardingSplitShardParams, | ||
) -> io::Result<()> { | ||
let mut store_update = self.store.trie_store().store_update(); | ||
let parent_shard_uid = split_shard_event.parent_shard; | ||
let parent_shard_uid_prefix = get_shard_uid_mapping(&self.store, parent_shard_uid); |
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.
If we do this in a loop until the last iteration returns the same value as the previous (meaning we navigated all children), would it be a good way to support any number of sub reshardings?
Although I don't see it likely going further down than shard grand children..
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.
I think it already works for any number of sub reshardings. E.g. split A -> B -> C, then we save to the db mapping from C to A. If we then split C -> D, then we would save to the db mapping from D to get_mapping(C)=A.
Is this what you meant?
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.
Yes.. I think it should work as you said. We'll be sure after adding tests :)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12688 +/- ##
=======================================
Coverage 70.53% 70.53%
=======================================
Files 847 847
Lines 172835 172839 +4
Branches 172835 172839 +4
=======================================
+ Hits 121901 121918 +17
+ Misses 45833 45821 -12
+ Partials 5101 5100 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM
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.
🚀
Introduce the fix from #12635 now, will add test later as it requires one of two: