-
Notifications
You must be signed in to change notification settings - Fork 665
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
feat(resharding) - Make shard ids non-contiguous - part 3 #12214
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12214 +/- ##
==========================================
+ Coverage 71.76% 71.82% +0.06%
==========================================
Files 825 827 +2
Lines 165868 166642 +774
Branches 165868 166642 +774
==========================================
+ Hits 119028 119685 +657
- Misses 41647 41743 +96
- Partials 5193 5214 +21
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.
🚀
for shard_uid in requested_shard_uids { | ||
if let Err(err) = flat_storage_manager.create_flat_storage_for_shard(*shard_uid) { | ||
for &(shard_index, shard_uid) in requested_shard_uids { | ||
if let Err(err) = flat_storage_manager.create_flat_storage_for_shard(shard_uid) { | ||
tracing::warn!(target: "state_snapshot", ?err, ?shard_uid, "Failed to create a flat storage for snapshot shard"); |
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.
nit: add shard_index
to log line
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'm avoiding doing that for now because then I would have to do it everywhere and in vast majority of places it doesn't matter.
@@ -177,7 +178,7 @@ impl ShardTries { | |||
pub fn create_state_snapshot( | |||
&self, | |||
prev_block_hash: CryptoHash, | |||
shard_uids: &[ShardUId], | |||
shard_uids: &[(ShardIndex, 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.
nit: shard_uids
doesn't fit 100% anymore as the parameter name
some mostly bad options: shards_indexes_and_uids
, shards_info
, shards_details
, ..
The next part of refactoring ShardId to newtype.
Organized by commits. The only non-trivial one should be the
state snapshot refactoring
. The interface in there is ugly because the state snapshot gets epoch manager captured inside of a lambda instead of directly. I didn't have the patience to properly refactor it, so I just added the shard indices to the lambda.