From 8111caebc2bb36ebad9c5be9738ad37ca963bd90 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Mon, 13 Nov 2023 11:40:41 +0200 Subject: refactor(tvix/store): remove from_url from PathInfoService trait 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 --- tvix/store/src/pathinfoservice/grpc.rs | 8 ++++---- tvix/store/src/pathinfoservice/memory.rs | 9 ++++----- tvix/store/src/pathinfoservice/mod.rs | 18 +----------------- tvix/store/src/pathinfoservice/sled.rs | 9 ++++----- 4 files changed, 13 insertions(+), 31 deletions(-) diff --git a/tvix/store/src/pathinfoservice/grpc.rs b/tvix/store/src/pathinfoservice/grpc.rs index 5363109efc28..7428830abd10 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, _directory_service: Arc, @@ -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, Error> { let path_info = self .grpc_client diff --git a/tvix/store/src/pathinfoservice/memory.rs b/tvix/store/src/pathinfoservice/memory.rs index d18453477a77..90a151fd04e5 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, directory_service: Arc, @@ -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, 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 af7bbc9f88e4..3fde10179b36 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, - directory_service: Arc, - ) -> Result - where - Self: Sized; - /// Retrieve a PathInfo message by the output digest. async fn get(&self, digest: [u8; 20]) -> Result, Error>; diff --git a/tvix/store/src/pathinfoservice/sled.rs b/tvix/store/src/pathinfoservice/sled.rs index fce0b7f44180..b389b14b95ad 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, directory_service: Arc, @@ -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, 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. -- cgit 1.4.1