Repository navigation
fix: make chaos routing reproducible from its seed - #862
Rich-T-kid wants to merge 3 commits into
Conversation
Signed-off-by: Richard Baah <[email protected]>
Rich-T-kid
changed the title
|
|
||
| fn shuffle_retry_urls<T>(urls: &mut [T], seed: Option<[u8; 32]>) { | ||
| match seed { | ||
| Some(seed) => urls.shuffle(&mut StdRng::from_seed(seed)), |
There was a problem hiding this comment.
Is it worth introducing these match statements on seed or could we just have a default randomly generated seed that's overridable and 1 code path?
There was a problem hiding this comment.
yea I guess, in practice wouldnt it just look like this? I'll do this in a seperate commit then we can compare
There was a problem hiding this comment.
doesn't seem to make must of an impact 88e6261
There was a problem hiding this comment.
the other match function too I meant to imply
alexanderbianchi
left a comment
alexanderbianchi
left a comment
There was a problem hiding this comment.
The nit isn't a big deal at all was just a thought, this looks reasonable
Signed-off-by: Richard Baah <[email protected]>
88e6261 to
080824c
Compare
Signed-off-by: Richard Baah <[email protected]>
| @@ -67,6 +67,9 @@ extensions_options! { | |||
| /// Maximum number of times the coordinator retries establishing a coordinator channel | |||
| /// after the initial dial fails with a retryable error. Set to 0 to disable retries. | |||
| pub max_coordinator_channel_retries: usize, default = 3 | |||
| /// Optional seed for the default random task-routing handler. When set, initial worker | |||
| /// selection and failover order are reproducible for the same stage and task numbers. | |||
| pub random_route_seed: Option<u64>, default = None | |||
There was a problem hiding this comment.
I think we should not have a public setting for this. Leaving it as an argument in RandomRouteTaskHandler::new(random_route_seed) should be enough right? You can call it directly from the test
Also a user may not use the RandomRouteTaskHandler. This setting is only for RandomRouteTaskHandler
| seed[..16].copy_from_slice(key.query_id.as_bytes()); | ||
| seed[16..24].copy_from_slice(&(key.stage_id as u64).to_le_bytes()); | ||
| let mut rng = StdRng::from_seed(seed); | ||
| let mut rng = StdRng::from_seed(routing_seed(key, seed, false)); |
There was a problem hiding this comment.
nit: I think you can calculate the routing_seed seed once in handle and pass it in
| seed[..16].copy_from_slice(key.query_id.as_bytes()); | ||
| seed[16..24].copy_from_slice(&(key.stage_id as u64).to_le_bytes()); | ||
| let mut rng = StdRng::from_seed(seed); | ||
| let mut rng = StdRng::from_seed(routing_seed(key, seed, false)); |
There was a problem hiding this comment.
Don't we want each tasks to each start at a different worker, so include_task should be true always? Then we don't need include_task if it's always true.
| let worker_resolver = LocalHostWorkerResolver::new(ports.clone()); | ||
| let mut distributed_config = DistributedConfig::default(); | ||
| distributed_config.max_coordinator_channel_retries = 5; | ||
| distributed_config.random_route_seed = Some(cfg.seed); |
There was a problem hiding this comment.
In the test worker service, do we take the task id into account in the worker?
You could have multiple tasks route to the same worker concurrently
worker 1: task 1 request
worker 1: task 2 request
and then the worker randomly decides if it should error or not. On subsequent test runs, it may throw an error for task 1 or 2 right? Because those requests are concurrent.
So maybe you want to make that more deterministic by putting the task id into the error decision.
There was a problem hiding this comment.
Or generate a num_errors per task which is shared across all workers. This would be how many errors should be thrown before returning success for task across all workers. Make sure to keep num_errors under the max allowed retries
Closes #811.
Problem
CHAOS_SEEDcontrols query batching and injected worker failures, but the default routing fallback still derives its initial worker from a random query ID and its failover order from an unseeded RNG. A rerun can therefore route tasks differently.Change
Add an optional
distributed.random_route_seed, unset by default. When configured, the existing fallback derives initial placement and failover order from that seed plus the stage and task numbers. The single-task colocation handlers keep their existing priority.The chaos test now uses its
CHAOS_SEEDfor routing as well as query generation and fault injection.Tests
Unit coverage checks that a fixed seed reproduces initial worker choices and failover order across different query IDs. The chaos test exercises the configured seed through the coordinator and workers.