refactor(resharding): renaming resharding code from "state split" to "resharding" #10393
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10393 +/- ##
==========================================
- Coverage 72.01% 71.99% -0.02%
==========================================
Files 718 718
Lines 144527 144520 -7
Branches 144527 144520 -7
==========================================
- Hits 104077 104047 -30
- Misses 35699 35718 +19
- Partials 4751 4755 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
chain/client-primitives/src/types.rs
Outdated
| @@ -133,8 +133,8 @@ impl ToString for ShardSyncStatus { | |||
| ShardSyncStatus::StateDownloadScheduling => "scheduling".to_string(), | |||
| ShardSyncStatus::StateDownloadApplying => "applying".to_string(), | |||
| ShardSyncStatus::StateDownloadComplete => "download complete".to_string(), | |||
| ShardSyncStatus::StateSplitScheduling => "split scheduling".to_string(), | |||
| ShardSyncStatus::StateSplitApplying => "split applying".to_string(), | |||
| ShardSyncStatus::ReshardingScheduling => "split scheduling".to_string(), | |||
There was a problem hiding this comment.
Is it safe to change the ToString value here? Say from "split scheduling" to "resharding scheduling"? I don't think we store this anywhere, but potentially might need to revisit logging/dashboards.
There was a problem hiding this comment.
Good catch, let's do this. I don't know of any dependencie. Either way we should be fine as long as nobody upgrades to this version in the middle of resharding which shouldn't happen anyway.
core/store/src/trie/state_parts.rs
Outdated
| @@ -580,7 +580,7 @@ mod tests { | |||
| (b"aaa".to_vec(), Some(vec![3; value_len])), | |||
| (b"aaaa".to_vec(), Some(vec![4; value_len])), | |||
| ]; | |||
| // We split state into `num_keys + 1` parts for convenience of testing, | |||
| // We resharding into `num_keys + 1` parts for convenience of testing, | |||
There was a problem hiding this comment.
nit: We reshard into...
| @@ -314,7 +314,7 @@ fn test_flat_storage_creation_start_from_state_part() { | |||
| let store = create_test_store(); | |||
|
|
|||
| // Process some blocks with flat storage. | |||
| // Split state into two parts and return trie keys corresponding to each part. | |||
| // resharding into two parts and return trie keys corresponding to each part. | |||
There was a problem hiding this comment.
nit: Reshard state into two parts...
The current naming is centered around "state split". It's really confusing to newcomers and even experienced developers as it's not obvious what spltting state pertains to. The new naming is centered around "resharding" which should be clear to anyone. Moreover in the future we may want to add more resharding ops, beyond splitting shards, so this refactoring makes it more future proof.
Care needs to be taken when handling the configs. Ideally this PR should be included in the same rollout as the resharding itself. Otherwise a node operator may configure resharding using the legacy "state_split_config" which will become obsolete as of this PR. cc @posvyatokum