refactor(tvix/store): remove from_url from PathInfoService trait r/7014

We don't gain much from making this part of the trait, it's still up to
`tvix_store::pathinfoservice::from_addr` to do most of the construction.

Move it out of the trait and into the specific *Service impls directly.

This allows further refactorings in followup CLs.

Change-Id: I99b93ef4acd83637a2f4888a1e586f1ca96390dc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10022
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>

4 files changed, 13 insertions, 31 deletions

diff --git a/tvix/store/src/pathinfoservice/grpc.rs b/tvix/store/src/pathinfoservice/grpc.rs
index 5363109efc..7428830abd 100644
--- a/tvix/store/src/pathinfoservice/grpc.rs
+++ b/tvix/store/src/pathinfoservice/grpc.rs
@@ -24,17 +24,14 @@ impl GRPCPathInfoService {
     ) -> Self {
         Self { grpc_client }
     }
-}
 
-#[async_trait]
-impl PathInfoService for GRPCPathInfoService {
     /// Constructs a [GRPCPathInfoService] from the passed [url::Url]:
     /// - scheme has to match `grpc+*://`.
     ///   That's normally grpc+unix for unix sockets, and grpc+http(s) for the HTTP counterparts.
     /// - In the case of unix sockets, there must be a path, but may not be a host.
     /// - In the case of non-unix sockets, there must be a host, but no path.
     /// The blob_service and directory_service arguments are ignored, because the gRPC service already provides answers to these questions.
-    fn from_url(
+    pub fn from_url(
         url: &url::Url,
         _blob_service: Arc<dyn BlobService>,
         _directory_service: Arc<dyn DirectoryService>,
@@ -44,7 +41,10 @@ impl PathInfoService for GRPCPathInfoService {
             proto::path_info_service_client::PathInfoServiceClient::new(channel),
         ))
     }
+}
 
+#[async_trait]
+impl PathInfoService for GRPCPathInfoService {
     async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
         let path_info = self
             .grpc_client
diff --git a/tvix/store/src/pathinfoservice/memory.rs b/tvix/store/src/pathinfoservice/memory.rs
index d18453477a..90a151fd04 100644
--- a/tvix/store/src/pathinfoservice/memory.rs
+++ b/tvix/store/src/pathinfoservice/memory.rs
@@ -29,15 +29,12 @@ impl MemoryPathInfoService {
             directory_service,
         }
     }
-}
 
-#[async_trait]
-impl PathInfoService for MemoryPathInfoService {
     /// Constructs a [MemoryPathInfoService] from the passed [url::Url]:
     /// - scheme has to be `memory://`
     /// - there may not be a host.
     /// - there may not be a path.
-    fn from_url(
+    pub fn from_url(
         url: &url::Url,
         blob_service: Arc<dyn BlobService>,
         directory_service: Arc<dyn DirectoryService>,
@@ -52,7 +49,10 @@ impl PathInfoService for MemoryPathInfoService {
 
         Ok(Self::new(blob_service, directory_service))
     }
+}
 
+#[async_trait]
+impl PathInfoService for MemoryPathInfoService {
     async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
         let db = self.db.read().unwrap();
 
@@ -113,7 +113,6 @@ mod tests {
     use crate::tests::utils::gen_directory_service;
 
     use super::MemoryPathInfoService;
-    use super::PathInfoService;
 
     /// This uses a wrong scheme.
     #[test]
diff --git a/tvix/store/src/pathinfoservice/mod.rs b/tvix/store/src/pathinfoservice/mod.rs
index af7bbc9f88..3fde10179b 100644
--- a/tvix/store/src/pathinfoservice/mod.rs
+++ b/tvix/store/src/pathinfoservice/mod.rs
@@ -3,13 +3,9 @@ mod grpc;
 mod memory;
 mod sled;
 
-use std::pin::Pin;
-use std::sync::Arc;
-
 use futures::Stream;
+use std::pin::Pin;
 use tonic::async_trait;
-use tvix_castore::blobservice::BlobService;
-use tvix_castore::directoryservice::DirectoryService;
 use tvix_castore::proto as castorepb;
 use tvix_castore::Error;
 
@@ -23,18 +19,6 @@ pub use self::sled::SledPathInfoService;
 /// The base trait all PathInfo services need to implement.
 #[async_trait]
 pub trait PathInfoService: Send + Sync {
-    /// Create a new instance by passing in a connection URL, as well
-    /// as instances of a [PathInfoService] and [DirectoryService] (as the
-    /// [PathInfoService] needs to talk to them).
-    /// TODO: check if we want to make this async, instead of lazily connecting
-    fn from_url(
-        url: &url::Url,
-        blob_service: Arc<dyn BlobService>,
-        directory_service: Arc<dyn DirectoryService>,
-    ) -> Result<Self, Error>
-    where
-        Self: Sized;
-
     /// Retrieve a PathInfo message by the output digest.
     async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error>;
 
diff --git a/tvix/store/src/pathinfoservice/sled.rs b/tvix/store/src/pathinfoservice/sled.rs
index fce0b7f441..b389b14b95 100644
--- a/tvix/store/src/pathinfoservice/sled.rs
+++ b/tvix/store/src/pathinfoservice/sled.rs
@@ -49,15 +49,12 @@ impl SledPathInfoService {
             directory_service,
         })
     }
-}
 
-#[async_trait]
-impl PathInfoService for SledPathInfoService {
     /// Constructs a [SledPathInfoService] from the passed [url::Url]:
     /// - scheme has to be `sled://`
     /// - there may not be a host.
     /// - a path to the sled needs to be provided (which may not be `/`).
-    fn from_url(
+    pub fn from_url(
         url: &url::Url,
         blob_service: Arc<dyn BlobService>,
         directory_service: Arc<dyn DirectoryService>,
@@ -86,7 +83,10 @@ impl PathInfoService for SledPathInfoService {
                 .map_err(|e| Error::StorageError(e.to_string()))
         }
     }
+}
 
+#[async_trait]
+impl PathInfoService for SledPathInfoService {
     async fn get(&self, digest: [u8; 20]) -> Result<Option<PathInfo>, Error> {
         match self.db.get(digest) {
             Ok(None) => Ok(None),
@@ -180,7 +180,6 @@ mod tests {
     use crate::tests::utils::gen_blob_service;
     use crate::tests::utils::gen_directory_service;
 
-    use super::PathInfoService;
     use super::SledPathInfoService;
 
     /// This uses a wrong scheme.