-
Notifications
You must be signed in to change notification settings - Fork 616
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): simple nayduck test for resharding #10162
Conversation
wacban
commented
Nov 13, 2023
- Added a simple nayduck test for resharding. It checks that the shard layout version and number of shards change at the right block.
- Added configuration for the delay while waiting for the state snapshot. Needed it, otherwise the test was all over the place.
- Refactored some common resharding test logic in nayduck.
// For colour decorators to work, they need to printed directly. Otherwise the decorators get escaped, garble output and don't add colours. | ||
debug!(target: "catchup", progress_per_shard = ?format_shard_sync_phase_per_shard(&shards_to_split, false), "Need to split states for shards"); |
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 log line doesn't make much sense.
- It is called multiple times, including after resharding is started but it doesn't actually know about the current status of it.
- There is a different log line just a bit later that prints the current status anyway.
// TODO(resharding) what happens to the shards_to_split here when | ||
// catchup_state_syncs already contains an entry for the sync hash? | ||
// Does it get overwritten? Are we guaranteed that the existing | ||
// entry contains the same data? |
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.
Checked that accidentaly. It's fine.
Self { | ||
batch_size: bytesize::ByteSize::kb(500), | ||
batch_delay: Duration::from_millis(100), | ||
retry_delay: Duration::from_secs(10), |
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.
30 sec?
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.
Fine with any value you find appropriate. I don't think it's going to matter too much as long as it's in the domain of a couple of seconds.