From 0f9ff4807f55401abb5b44b25e214974c59044e1 Mon Sep 17 00:00:00 2001 From: Nikolay Komarevskiy Date: Mon, 20 Jan 2025 18:18:42 +0100 Subject: [PATCH] chore: use Option for healthy routes count --- ic-agent/src/agent/route_provider.rs | 18 +++++++++--------- .../dynamic_routing/dynamic_route_provider.rs | 14 +++++++------- .../snapshot/latency_based_routing.rs | 14 +++++++------- .../snapshot/round_robin_routing.rs | 4 ++-- .../snapshot/routing_snapshot.rs | 9 +++++++-- 5 files changed, 32 insertions(+), 27 deletions(-) diff --git a/ic-agent/src/agent/route_provider.rs b/ic-agent/src/agent/route_provider.rs index 7d7d9f06..77225a64 100644 --- a/ic-agent/src/agent/route_provider.rs +++ b/ic-agent/src/agent/route_provider.rs @@ -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); } /// A simple implementation of the [`RouteProvider`] which produces an even distribution of the urls from the input ones. @@ -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) { + (self.routes.len(), None) } } @@ -146,8 +146,8 @@ impl RouteProvider for Url { fn n_ordered_routes(&self, _: usize) -> Result, AgentError> { Ok(vec![self.route()?]) } - fn routes_stats(&self) -> (usize, usize) { - (1, 1) + fn routes_stats(&self) -> (usize, Option) { + (1, None) } } @@ -231,7 +231,7 @@ impl RouteProvider for DynamicRouteProvider { fn n_ordered_routes(&self, n: usize) -> Result, AgentError> { self.inner.n_ordered_routes(n) } - fn routes_stats(&self) -> (usize, usize) { + fn routes_stats(&self) -> (usize, Option) { self.inner.routes_stats() } } @@ -289,8 +289,8 @@ impl RouteProvider for UrlUntilReady { self.url.route() } } - fn routes_stats(&self) -> (usize, usize) { - (1, 1) + fn routes_stats(&self) -> (usize, Option) { + (1, None) } } diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs index 4ec96476..dca44562 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/dynamic_route_provider.rs @@ -187,7 +187,7 @@ where Ok(urls) } - fn routes_stats(&self) -> (usize, usize) { + fn routes_stats(&self) -> (usize, Option) { let snapshot = self.routing_snapshot.load(); snapshot.nodes_stats() } @@ -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. @@ -437,7 +437,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 3: multiple route() calls return 2 different domains with equal fairness (repetition). // One node is set to unhealthy. @@ -445,7 +445,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(), 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. @@ -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. @@ -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. @@ -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] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs index 4127d796..87b3b37e 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/latency_based_routing.rs @@ -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) { + (self.existing_nodes.len(), Some(self.healthy_nodes.len())) } } @@ -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] @@ -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] @@ -384,7 +384,7 @@ 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); @@ -392,7 +392,7 @@ mod tests { 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); @@ -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] diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs index 93332b7d..28c06fdf 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/round_robin_routing.rs @@ -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) { + (self.existing_nodes.len(), Some(self.healthy_nodes.len())) } } diff --git a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs index f5e7ef54..89191137 100644 --- a/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs +++ b/ic-agent/src/agent/route_provider/dynamic_routing/snapshot/routing_snapshot.rs @@ -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); }