Skip to content

Commit

Permalink
code review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@wedamija
wedamija committed Jan 3, 2025
1 parent 49ae78f commit c93952d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 50 deletions.
4 changes: 1 addition & 3 deletions src/scheduler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,8 @@ async fn scheduler_loop(

let mut results = vec![];

// TODO(epurkhiser): Check if we skipped any ticks If we did we should catch up on those.

for config in configs {
if config.should_run(tick, region.clone()) {
if config.should_run(tick, &region) {
results.push(queue_check(&executor_sender, tick, config));
} else {
tracing::debug!(%config.subscription_id, %tick, "scheduler.skipped_config");
Expand Down
94 changes: 47 additions & 47 deletions src/types/check_config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ impl CheckConfig {
.collect()
}

pub fn should_run(&self, tick: Tick, current_region: String) -> bool {
pub fn should_run(&self, tick: Tick, current_region: &String) -> bool {
// Determines if this check should run in the current region at this current tick

// If region configs aren't present, assume we should always run this check
Expand All @@ -107,7 +107,7 @@ impl CheckConfig {
let running_region_idx = (tick.time().timestamp() / self.interval as i64)
.checked_rem(active_regions.len() as i64)
.unwrap();
active_regions[running_region_idx as usize] == current_region
&active_regions[running_region_idx as usize] == current_region
}
}

Expand Down Expand Up @@ -281,17 +281,17 @@ mod tests {

// We should always run if regions aren't configured
let no_region_config = CheckConfig::default();
assert!(no_region_config.should_run(tick, "us_west".to_string()));
assert!(no_region_config.should_run(tick, &"us_west".to_string()));
let no_active_regions_config = CheckConfig {
region_schedule_mode: Some(RegionScheduleMode::RoundRobin),
..Default::default()
};
assert!(no_active_regions_config.should_run(tick, "us_west".to_string()));
assert!(no_active_regions_config.should_run(tick, &"us_west".to_string()));
let no_schedule_mode_config = CheckConfig {
active_regions: Some(vec!["us_west".to_string(), "us_east".to_string()]),
..Default::default()
};
assert!(no_schedule_mode_config.should_run(tick, "us_west".to_string()));
assert!(no_schedule_mode_config.should_run(tick, &"us_west".to_string()));

// When there's just one region we should also always run
let one_region_config = CheckConfig {
Expand All @@ -301,21 +301,21 @@ mod tests {
};
assert!(one_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(1, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(one_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(61, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));

// If configured region doesn't match at all we should never run
assert!(!one_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(1, 0).unwrap()),
"other_region".to_string()
&"other_region".to_string()
));
assert!(!one_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(61, 0).unwrap()),
"other_region".to_string()
&"other_region".to_string()
));

let multi_region_config = CheckConfig {
Expand All @@ -330,96 +330,96 @@ mod tests {
// With 3 regions configured, the US west region should only run once every 3 minutes
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(1, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(61, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(121, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(181, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
// Check other regions run
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(61, 0).unwrap()),
"us_east".to_string()
&"us_east".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(121, 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));

// Try different spots in the minute
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(29, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(59, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(83, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(83, 0).unwrap()),
"us_east".to_string()
&"us_east".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(119, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(119, 0).unwrap()),
"us_east".to_string()
&"us_east".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(145, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(145, 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(171, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(171, 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(220, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(200, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));

// Try a more complicated start minute
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp(60 * (3 * 12345), 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp((60 * (3 * 12345)) + 60, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp((60 * (3 * 12345)) + 120, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config.should_run(
Tick::from_time(DateTime::from_timestamp((60 * (3 * 12345)) + 180, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));

// Try a larger interval to be sure things work
Expand All @@ -436,70 +436,70 @@ mod tests {
// With 3 regions configured, the US west region should only run once every hour
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(0, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(60 * 25, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(60 * 25, 0).unwrap()),
"us_east".to_string()
&"us_east".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(60 * 50, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(60 * 50, 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(60 * 71, 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));

// Try more complicated times
// ((60 * 60) * 13423) Is just selecting a random large hour, then we add minutes to it
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 15), 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 39), 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 39), 0).unwrap()),
"us_east".to_string()
&"us_east".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 40), 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 40), 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 59), 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 59), 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));
assert!(multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 60), 0).unwrap()),
"us_west".to_string()
&"us_west".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 60), 0).unwrap()),
"us_east".to_string()
&"us_east".to_string()
));
assert!(!multi_region_config_large_interval.should_run(
Tick::from_time(DateTime::from_timestamp(((60 * 60) * 13423) + (60 * 60), 0).unwrap()),
"eu_west".to_string()
&"eu_west".to_string()
));
}
}

0 comments on commit c93952d

Please sign in to comment.