Skip to content

Commit

Permalink
chore: use Option<usize> for healthy routes count
Browse files Browse the repository at this point in the history
  • Loading branch information
nikolay-komarevskiy committed Jan 20, 2025
1 parent a633622 commit 0f9ff48
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 27 deletions.
18 changes: 9 additions & 9 deletions ic-agent/src/agent/route_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ pub trait RouteProvider: std::fmt::Debug + Send + Sync {
/// Returns the total number of routes and healthy routes as a tuple.
///
/// - First element is the total number of routes available (both healthy and unhealthy)
/// - Second element is the number of currently healthy routes
/// - Second element is the number of currently healthy routes, or None if health status information is unavailable
///
/// A healthy route is one that is available and ready to receive traffic.
/// The specific criteria for what constitutes a "healthy" route is implementation dependent.
fn routes_stats(&self) -> (usize, usize);
fn routes_stats(&self) -> (usize, Option<usize>);
}

/// A simple implementation of the [`RouteProvider`] which produces an even distribution of the urls from the input ones.
Expand Down Expand Up @@ -104,8 +104,8 @@ impl RouteProvider for RoundRobinRouteProvider {
Ok(urls)
}

fn routes_stats(&self) -> (usize, usize) {
(self.routes.len(), self.routes.len())
fn routes_stats(&self) -> (usize, Option<usize>) {
(self.routes.len(), None)
}
}

Expand Down Expand Up @@ -146,8 +146,8 @@ impl RouteProvider for Url {
fn n_ordered_routes(&self, _: usize) -> Result<Vec<Url>, AgentError> {
Ok(vec![self.route()?])
}
fn routes_stats(&self) -> (usize, usize) {
(1, 1)
fn routes_stats(&self) -> (usize, Option<usize>) {
(1, None)
}
}

Expand Down Expand Up @@ -231,7 +231,7 @@ impl RouteProvider for DynamicRouteProvider {
fn n_ordered_routes(&self, n: usize) -> Result<Vec<Url>, AgentError> {
self.inner.n_ordered_routes(n)
}
fn routes_stats(&self) -> (usize, usize) {
fn routes_stats(&self) -> (usize, Option<usize>) {
self.inner.routes_stats()
}
}
Expand Down Expand Up @@ -289,8 +289,8 @@ impl<R: RouteProvider> RouteProvider for UrlUntilReady<R> {
self.url.route()
}
}
fn routes_stats(&self) -> (usize, usize) {
(1, 1)
fn routes_stats(&self) -> (usize, Option<usize>) {
(1, None)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ where
Ok(urls)
}

fn routes_stats(&self) -> (usize, usize) {
fn routes_stats(&self) -> (usize, Option<usize>) {
let snapshot = self.routing_snapshot.load();
snapshot.nodes_stats()
}
Expand Down Expand Up @@ -422,7 +422,7 @@ mod tests {
tokio::time::sleep(snapshot_update_duration).await;
let routed_domains = route_n_times(6, Arc::clone(&route_provider));
assert_routed_domains(routed_domains, vec![node_1.domain()], 6);
assert_eq!(route_provider.routes_stats(), (1, 1));
assert_eq!(route_provider.routes_stats(), (1, Some(1)));

// Test 2: multiple route() calls return 3 different domains with equal fairness (repetition).
// Two healthy nodes are added to the topology.
Expand All @@ -437,15 +437,15 @@ mod tests {
vec![node_1.domain(), node_2.domain(), node_3.domain()],
2,
);
assert_eq!(route_provider.routes_stats(), (3, 3));
assert_eq!(route_provider.routes_stats(), (3, Some(3)));

// Test 3: multiple route() calls return 2 different domains with equal fairness (repetition).
// One node is set to unhealthy.
checker.overwrite_healthy_nodes(vec![node_1.clone(), node_3.clone()]);
tokio::time::sleep(snapshot_update_duration).await;
let routed_domains = route_n_times(6, Arc::clone(&route_provider));
assert_routed_domains(routed_domains, vec![node_1.domain(), node_3.domain()], 3);
assert_eq!(route_provider.routes_stats(), (3, 2));
assert_eq!(route_provider.routes_stats(), (3, Some(2)));

// Test 4: multiple route() calls return 3 different domains with equal fairness (repetition).
// Unhealthy node is set back to healthy.
Expand All @@ -457,7 +457,7 @@ mod tests {
vec![node_1.domain(), node_2.domain(), node_3.domain()],
2,
);
assert_eq!(route_provider.routes_stats(), (3, 3));
assert_eq!(route_provider.routes_stats(), (3, Some(3)));

// Test 5: multiple route() calls return 3 different domains with equal fairness (repetition).
// One healthy node is added, but another one goes unhealthy.
Expand All @@ -476,7 +476,7 @@ mod tests {
vec![node_2.domain(), node_3.domain(), node_4.domain()],
2,
);
assert_eq!(route_provider.routes_stats(), (4, 3));
assert_eq!(route_provider.routes_stats(), (4, Some(3)));

// Test 6: multiple route() calls return a single domain=api1.com.
// One node is set to unhealthy and one is removed from the topology.
Expand All @@ -485,7 +485,7 @@ mod tests {
tokio::time::sleep(snapshot_update_duration).await;
let routed_domains = route_n_times(3, Arc::clone(&route_provider));
assert_routed_domains(routed_domains, vec![node_2.domain()], 3);
assert_eq!(route_provider.routes_stats(), (3, 1));
assert_eq!(route_provider.routes_stats(), (3, Some(1)));
}

#[tokio::test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ impl RoutingSnapshot for LatencyRoutingSnapshot {
true
}

fn nodes_stats(&self) -> (usize, usize) {
(self.existing_nodes.len(), self.healthy_nodes.len())
fn nodes_stats(&self) -> (usize, Option<usize>) {
(self.existing_nodes.len(), Some(self.healthy_nodes.len()))
}
}

Expand Down Expand Up @@ -356,7 +356,7 @@ mod tests {
assert!(!snapshot.has_nodes());
assert!(snapshot.next_node().is_none());
assert!(snapshot.next_n_nodes(1).is_empty());
assert_eq!(snapshot.nodes_stats(), (0, 0));
assert_eq!(snapshot.nodes_stats(), (0, Some(0)));
}

#[test]
Expand All @@ -372,7 +372,7 @@ mod tests {
assert!(snapshot.nodes_with_metrics.is_empty());
assert!(!snapshot.has_nodes());
assert!(snapshot.next_node().is_none());
assert_eq!(snapshot.nodes_stats(), (0, 0));
assert_eq!(snapshot.nodes_stats(), (0, Some(0)));
}

#[test]
Expand All @@ -384,15 +384,15 @@ mod tests {
let node = Node::new("api1.com").unwrap();
let health = HealthCheckStatus::new(Some(Duration::from_secs(1)));
snapshot.existing_nodes.insert(node.clone());
assert_eq!(snapshot.nodes_stats(), (1, 0));
assert_eq!(snapshot.nodes_stats(), (1, Some(0)));
// Check first update
let is_updated = snapshot.update_node(&node, health);
assert!(is_updated);
assert!(snapshot.has_nodes());
let node_with_metrics = snapshot.nodes_with_metrics.first().unwrap();
assert_eq!(node_with_metrics.score, (2.0 / 1.0) / 2.0);
assert_eq!(snapshot.next_node().unwrap(), node);
assert_eq!(snapshot.nodes_stats(), (1, 1));
assert_eq!(snapshot.nodes_stats(), (1, Some(1)));
// Check second update
let health = HealthCheckStatus::new(Some(Duration::from_secs(2)));
let is_updated = snapshot.update_node(&node, health);
Expand All @@ -415,7 +415,7 @@ mod tests {
assert_eq!(snapshot.nodes_with_metrics.len(), 1);
assert_eq!(snapshot.existing_nodes.len(), 1);
assert!(snapshot.next_node().is_none());
assert_eq!(snapshot.nodes_stats(), (1, 0));
assert_eq!(snapshot.nodes_stats(), (1, Some(0)));
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ impl RoutingSnapshot for RoundRobinRoutingSnapshot {
}
}

fn nodes_stats(&self) -> (usize, usize) {
(self.existing_nodes.len(), self.healthy_nodes.len())
fn nodes_stats(&self) -> (usize, Option<usize>) {
(self.existing_nodes.len(), Some(self.healthy_nodes.len()))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ pub trait RoutingSnapshot: Send + Sync + Clone + Debug {
fn sync_nodes(&mut self, nodes: &[Node]) -> bool;
/// Updates the health status of a specific node, returning `true` if the node was found and updated.
fn update_node(&mut self, node: &Node, health: HealthCheckStatus) -> bool;
/// Returns the total number of nodes and healthy nodes as a tuple with first and second element, respectively.
fn nodes_stats(&self) -> (usize, usize);
/// Returns the total number of nodes and healthy nodes as a tuple.
///
/// - First element is the total number of nodes available (both healthy and unhealthy)
/// - Second element is the number of currently healthy nodes, or None if health status information is unavailable
///
/// The specific criteria for what constitutes a "healthy" node is implementation dependent.
fn nodes_stats(&self) -> (usize, Option<usize>);
}

0 comments on commit 0f9ff48

Please sign in to comment.