feat(BOUN-1156): Integration of Discovery Library #3

nikolay-komarevskiy · 2024-06-03T09:12:09Z

Incorporate HealthCheckRouteProvider to ic-gateway for dynamic routing mode.

blind-oracle · 2024-06-06T06:58:56Z

src/cli.rs

+
+    /// Whether to use static URLs or dynamically discovered URLs for routing.
+    /// For the dynamic routing case, provided argument ic-url: Vec<Url> is used as a seed list of API Nodes.
+    #[clap(long = "use_dynamic_routing")]


Change to ic-use-dynamic-routing or better ic-use-discovery maybe...

blind-oracle · 2024-06-06T08:43:03Z

src/routing/ic/health_check.rs

+const SERVICE_NAME: &str = "HealthCheckImpl";
+
+#[derive(Debug)]
+pub struct HealthCheckImpl {


I'd call that HealthChecker or smth like this, I don't understand those -Impl suffixes :)

src/routing/ic/health_check.rs

blind-oracle · 2024-06-06T08:47:46Z

src/routing/ic/transport.rs

+                .map_err(|err| TransportProviderError::UnableToGetTransport(err.to_string()))?,
+        );
+
+        Ok(transport as Arc<dyn Transport>)


as Arc<dyn Transport> isn't needed here I think

blind-oracle · 2024-06-06T10:06:39Z

src/tasks.rs

+    S: Send + Sync + Debug + Clone + Snapshot + 'static,
+{
+    async fn run(&self, token: CancellationToken) -> Result<(), Error> {
+        self.0.run().await;


It will get stuck on this await forever and will not stop when the token is cancelled. You need select! here.

I thought about using select! when implementing it, but it felt unnecessary. self.0.run().await; is designed to return immediately. So the token cancellation will be awaited right away and will let the stop() be called after.

i can put select! though to make it even more robust

on the other hand, calling self.0.stop() before this nearly instantself.0.run().await finishes can be IMO ungraceful, as this is rather start() actually

if run() does not block on await then it's ok as-is, I thought it just blocks there.

blind-oracle · 2024-06-06T12:09:30Z

src/tasks.rs

@@ -63,3 +64,18 @@ impl Run for ocsp_stapler::Stapler {
        Ok(())
    }
 }
+
+pub struct TaskRouteProvider<S>(pub Arc<HealthCheckRouteProvider<S>>);


Btw do we even need a newtype around it? Maybe just implement Run directly?

not necessary indeed, i thought having the same run() method would be confusing for the compiler, but it seems not

blind-oracle · 2024-06-06T19:13:24Z

src/routing/mod.rs

+            return Err(anyhow!("Seed list of API Nodes can't be empty"));
+        }
+
+        let route_provider = {


Can we maybe move this stuff along with the constants somewhere to the ic module? To keep setup_router() shorter and cleaner.

blind-oracle · 2024-06-06T19:15:51Z

src/routing/ic/transport.rs

+        let route_provider = Arc::new(
+            RoundRobinRouteProvider::new(vec![url.as_str()])
+                .map_err(|err| TransportProviderError::UnableToGetTransport(err.to_string()))?,
+        ) as Arc<dyn RouteProvider>;


Again as Arc<dyn RouteProvider> isn't needed, Rust is smart enough to figure out :)

clippy is needed here

blind-oracle · 2024-06-07T17:53:58Z

Hey I guess let's merge it, though I made some changes to the router but I guess it should merge w/o conflicts.

nikolay-komarevskiy force-pushed the BOUN-1156-integrate-discovery-library branch 3 times, most recently from acd7193 to 78af939 Compare June 3, 2024 18:26

nikolay-komarevskiy added 2 commits June 5, 2024 14:02

add TaskRouteProvider

090bf37

incorporate HealthCheckRouteProvider

3ca5234

nikolay-komarevskiy force-pushed the BOUN-1156-integrate-discovery-library branch from 78af939 to 3ca5234 Compare June 5, 2024 15:33

print urls

4abcc1d

nikolay-komarevskiy marked this pull request as ready for review June 5, 2024 15:51