Skip to content

Commit 78cf304

Browse files
committed
default_policy: LA tests: don't rely on timing
The test bearing an undeniably attractive name: `latency_aware_default_ policy_stops_penalising_after_min_average_increases_enough_only_after_ update_rate_elapses` has shown to be flaky. I suspect it is due to the fact that it relies on timing to be valid, and execution in CI is often extremely slow. The problem, most probably, lies in minimum average updates done periodically: the test code assumes that an update has not yet been triggered at some point, which in CI happens to not be true. To combat that, those tests are changed to use the MinAvgUpdater explicitly instead of scheduling its periodic runs. This prevents flakiness.
1 parent 4ebfda8 commit 78cf304

File tree

1 file changed

+40
-9
lines changed

1 file changed

+40
-9
lines changed

scylla/src/transport/load_balancing/default.rs

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1837,6 +1837,22 @@ mod latency_awareness {
18371837
minimum_measurements,
18381838
)
18391839
}
1840+
1841+
#[cfg(test)]
1842+
fn build_for_test(self) -> (LatencyAwareness, MinAvgUpdater) {
1843+
let Self {
1844+
exclusion_threshold,
1845+
retry_period,
1846+
update_rate,
1847+
minimum_measurements,
1848+
} = self;
1849+
LatencyAwareness::new_for_test(
1850+
exclusion_threshold,
1851+
retry_period,
1852+
update_rate,
1853+
minimum_measurements,
1854+
)
1855+
}
18401856
}
18411857

18421858
impl Default for LatencyAwarenessBuilder {
@@ -2039,6 +2055,21 @@ mod latency_awareness {
20392055
latency_aware_default_policy_customised(|b| b)
20402056
}
20412057

2058+
fn latency_aware_policy_with_explicit_updater_customised(
2059+
configurer: impl FnOnce(LatencyAwarenessBuilder) -> LatencyAwarenessBuilder,
2060+
) -> (DefaultPolicy, MinAvgUpdater) {
2061+
let (latency_awareness, updater) =
2062+
configurer(LatencyAwareness::builder()).build_for_test();
2063+
(
2064+
default_policy_with_given_latency_awareness(latency_awareness),
2065+
updater,
2066+
)
2067+
}
2068+
2069+
fn latency_aware_default_policy_with_explicit_updater() -> (DefaultPolicy, MinAvgUpdater) {
2070+
latency_aware_policy_with_explicit_updater_customised(|b| b)
2071+
}
2072+
20422073
#[tokio::test]
20432074
async fn latency_aware_default_policy_does_not_penalise_if_no_latency_info_available_yet() {
20442075
let policy = latency_aware_default_policy();
@@ -2248,7 +2279,7 @@ mod latency_awareness {
22482279
.with_env_filter(tracing_subscriber::EnvFilter::from_default_env())
22492280
.without_time()
22502281
.try_init();
2251-
let policy = latency_aware_default_policy();
2282+
let (policy, updater) = latency_aware_default_policy_with_explicit_updater();
22522283
let cluster = tests::mock_cluster_data_for_token_unaware_tests().await;
22532284

22542285
let min_avg = Duration::from_millis(10);
@@ -2313,8 +2344,7 @@ mod latency_awareness {
23132344
);
23142345

23152346
// Await last min average updater.
2316-
// policy.latency_awareness.as_ref().unwrap().refresh_last_min_avg_nodes(&cluster.all_nodes); FIXME:
2317-
tokio::time::sleep(policy.latency_awareness.as_ref().unwrap()._update_rate * 5).await;
2347+
updater.tick().await;
23182348

23192349
let expected_groups = ExpectedGroupsBuilder::new()
23202350
.group([2, 3]) // pick + fallback local nodes
@@ -2335,7 +2365,8 @@ mod latency_awareness {
23352365
#[tokio::test]
23362366
async fn latency_aware_default_policy_stops_penalising_after_min_average_increases_enough_only_after_update_rate_elapses(
23372367
) {
2338-
let policy = latency_aware_default_policy();
2368+
let (policy, updater) = latency_aware_default_policy_with_explicit_updater();
2369+
23392370
let cluster = tests::mock_cluster_data_for_token_unaware_tests().await;
23402371

23412372
let min_avg = Duration::from_millis(10);
@@ -2379,7 +2410,7 @@ mod latency_awareness {
23792410
);
23802411

23812412
// Await last min average updater.
2382-
tokio::time::sleep(policy.latency_awareness.as_ref().unwrap()._update_rate).await;
2413+
updater.tick().await;
23832414
{
23842415
// min_avg is low enough to penalise node 1
23852416
let expected_groups = ExpectedGroupsBuilder::new()
@@ -2437,7 +2468,7 @@ mod latency_awareness {
24372468
.await;
24382469
}
24392470

2440-
tokio::time::sleep(policy.latency_awareness.as_ref().unwrap()._update_rate).await;
2471+
updater.tick().await;
24412472
{
24422473
// min_avg has been updated and is already high enough to stop penalising node 1
24432474
let expected_groups = ExpectedGroupsBuilder::new()
@@ -2618,7 +2649,7 @@ mod latency_awareness {
26182649
];
26192650

26202651
for test in &tests {
2621-
let policy = latency_aware_default_policy();
2652+
let (policy, updater) = latency_aware_default_policy_with_explicit_updater();
26222653

26232654
if let Some(preset_min_avg) = test.preset_min_avg {
26242655
policy.set_nodes_latency_stats(
@@ -2633,14 +2664,14 @@ mod latency_awareness {
26332664
)],
26342665
);
26352666
// Await last min average updater for update with a forged min_avg.
2636-
tokio::time::sleep(latency_awareness_defaults._update_rate).await;
2667+
updater.tick().await;
26372668
policy.set_nodes_latency_stats(&cluster, &[(1, None)]);
26382669
}
26392670
policy.set_nodes_latency_stats(&cluster, test.latency_stats);
26402671

26412672
if test.preset_min_avg.is_none() {
26422673
// Await last min average updater for update with None min_avg.
2643-
tokio::time::sleep(latency_awareness_defaults._update_rate).await;
2674+
updater.tick().await;
26442675
}
26452676

26462677
test_default_policy_with_given_cluster_and_routing_info(

0 commit comments

Comments
 (0)