-
Notifications
You must be signed in to change notification settings - Fork 622
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): flat storage resharding children catchup #12312
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
fn handle_memtrie_reload(&self, _shard_uid: ShardUId) { | ||
// TODO(resharding) |
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 thought this may be the entrypoint for memtrie rebuild.
@@ -1514,4 +1765,200 @@ mod tests { | |||
); | |||
assert_eq!(flat_store.get(right_child_shard, &buffered_receipt_key), Ok(None)); | |||
} | |||
|
|||
/// Base test scenario for testing children catchup. | |||
fn children_catchup_base(with_restart: bool) { |
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 most relevant change in tests is .. the addition of this test.
merged_changes.merge(changes); | ||
store_update.remove_delta(shard_uid, flat_head_block_hash); | ||
} | ||
// TODO (resharding): if flat_head_block_hash == state sync hash -> do snapshot |
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.
@marcelo-gonzalez This one is the designated point to trigger the snapshot. We can discuss how it can be done in practice, the important consideration is that here the flat storage has all the state needed.
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.
Ideally we should do this in client. This is perhaps not the best place due to potential race conditions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12312 +/- ##
==========================================
+ Coverage 71.24% 71.29% +0.05%
==========================================
Files 838 838
Lines 169346 169705 +359
Branches 169346 169705 +359
==========================================
+ Hits 120651 120998 +347
+ Misses 43449 43446 -3
- Partials 5246 5261 +15
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.
Would love others to take a look as well
// Shard catchup task is delayed and could get postponed several times. This must be | ||
// done to cover the scenario in which catchup is triggered so fast that the initial | ||
// state of the new flat storage is beyond the chain final tip. | ||
ctx.run_later( |
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.
This isn't how run_later
works. The only thing the current code does is call the handle_flat_storage_shard_catchup
after 100 ms.
What we ideally want is something like
- Check some condition to see if we can run
handle_flat_storage_shard_catchup
. If true, then just runhandle_flat_storage_shard_catchup
and return - If false, then call
ctx.run_later
recursively so that the same condition can be checked again later.
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.
Maybe run_later
is misused here. This has been my tentative solution to the following problem.
Originally, I was calling directly handle_flat_storage_shard_catchup
and waiting some time inside the function. But it was failing in test loop for resharding_v3 because the test was getting stuck inside this function call. The other actors weren't progressing at all and the condition to resume catchup wasn't satisfied ever.
Basically I need a way to postpone execution of catchup until the canonical chain makes enough progress. I found that run_later
achieves that by allowing other actors to make progress in the meantime.
Should I rather change the test_loop test to avoid this problem?
Please feel free to suggest a better approach (even offline)
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.
This is an example of how run_later is used. We can discuss this offline
&self, | ||
shard_uid: ShardUId, | ||
flat_head_block_hash: CryptoHash, | ||
chain_store: &ChainStore, |
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.
Aghh... another instance of ChainStore being passed around! :(
Now I really want to land #12159 soon!
(Just personal rant)
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'd love to not have to pass this chain_store thingy around 🤣
merged_changes.apply_to_flat_state(&mut store_update, shard_uid); | ||
store_update.set_flat_storage_status( | ||
shard_uid, | ||
FlatStorageStatus::Resharding(FlatStorageReshardingStatus::CatchingUp( |
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.
Quick sanity check, when do we set the status to CatchingUp? Should this be at the beginning of the apply_deltas function or here? Also, shouldn't we do this once instead of doing it for each batch?
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.
We set status to CatchingUp the first time in split_shard_task_postprocessing
after all key values have been split successfully.
Then we do it in each batch to update the block has of the flat head.
}; | ||
if height > chain_final_head.height { | ||
info!(target = "resharding", ?height, chain_final_height = ?chain_final_head.height, "flat head beyond chain final tip: postponing flat storage shard catchup task"); | ||
self.scheduler.send(ReshardingRequest::FlatStorageShardCatchup { |
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.
Wait, if this check is saying height is already beyond final head, why are we calling the schedule here?
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.
Oh this is a surprise I got from test loop resharding test.
It shouldn't happen in practice but the logic is the following:
If the split operation is very fast, since we split parent flat head and deltas together, the children flat storage after the split may have their flat head beyond the final block height. We don't want that because of the invariant that flat head <= final block height, so we delay the catchup until the canonical chain contains the children flat head hash.
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.
Hmm, I see, well, I don't think this should be the solution. Instead what we should do is delay and retry the original split flat storage request (the run_later logic) for FlatStorageSplitShard and then we can hopefully remove this code and the retry logic for FlatStorageShardCatchup
(assuming FlatStorageShardCatchup is called AFTER split is completed)
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.
*retry when height <= chain head height
let mut deltas_gc_count = 0; | ||
for delta_metadata in deltas_metadata { | ||
if delta_metadata.block.height <= flat_head.height { | ||
store_update.remove_delta(shard_uid, delta_metadata.block.hash); |
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 would like @Longarithm to take a look at this function and check whether it looks good
|
||
/// Dedicated actor for resharding V3. | ||
pub struct ReshardingActor {} | ||
pub struct ReshardingActor { | ||
chain_store: ChainStore, |
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.
This doesn't look right. If resharder is the entity that requires the chain store, shouldn't it hold chain store instead of ReshardingActor?
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.
Originally I thought chain_store might be used outside of resharder. If passing it around is not too complicated I can move it inside resharder
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 good at a first glance, I added a few comments for now.
)), | ||
); | ||
self.scheduler.send(ReshardingRequest::FlatStorageShardCatchup { |
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.
Is this going from the resharding actor to itself?
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.
It becomes really really hard to figure out which of these functions are called from within resharding actor and which aren't. That's why I wanted us to keep all functions executed in the actor as part of the actor instead of here.
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.
Even just for this review, I had to bounce around several times to see who is calling who and when does a resharding request get converted to a function call in flat_storage_resharder
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.
Yeah now that you mentioned it I also struggled to grasp what's happening where. Do you think this should be addressed in this PR or as a follow up? @shreyan-gupta @Trisfald
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 we can follow up in a separate PR, try to get this in first
// If the flat head is not in the canonical chain this task has failed. | ||
match chain_store.get_block_hash_by_height(height) { |
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.
That isn't what I expected the check for canonical to look like but alright :)
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.
What were the alternatives @wacban?
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.
Oh I don't know, it just surprised me. There is a function call is_on_current_chain
in chain that seems to work in a similar way.
let new_account_left_child = account!(format!("oo{}", height)); | ||
let state_changes_left_child = vec![RawStateChangesWithTrieKey { | ||
trie_key: TrieKey::Account { account_id: new_account_left_child.clone() }, | ||
changes: vec![RawStateChange { | ||
cause: StateChangeCause::InitialState, | ||
data: Some(new_account_left_child.as_bytes().to_vec()), | ||
}], | ||
}]; | ||
manager | ||
.save_flat_state_changes( | ||
block_hash, | ||
prev_hash, | ||
height, | ||
left_child_shard, | ||
&state_changes_left_child, | ||
) | ||
.unwrap() | ||
.commit() | ||
.unwrap(); |
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: This is copy pasted, can you refactor to a method?
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.
Done!
// Shard catchup task is delayed and could get postponed several times. This must be | ||
// done to cover the scenario in which catchup is triggered so fast that the initial | ||
// state of the new flat storage is beyond the chain final tip. |
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.
omg, thanks for the comment
I have address the most immediate feedback in the PR. As discussed offline, major changes such as delaying the split shard task instead of the catchup task will be done in another PR. |
PR to add children catchup step for flat storages created as a result of a parent shard split.
In previous iterations, the two children shards were populated in a background task from the flat storage of the parent at height
last block of old shard layout
(post-processing).Since the task mentioned above takes a long time and the children are active shards in the
first block of the new shard layout
their flat storage accumulates a lot of deltas.The catchup step applies delta in the background, then finalizes creation of child flat storage, and triggers a possible memtrie rebuild.
Part of #12174