From 045048f7dca130eaaabf69f7a3c305b27f08a188 Mon Sep 17 00:00:00 2001 From: Zeljko Mihaljcic <7150613+zehiko@users.noreply.github.com> Date: Mon, 4 Nov 2024 16:25:05 +0100 Subject: [PATCH] Remote store registration API only registers a single recording (#7982) --- .../proto/rerun/v0/remote_store.proto | 16 ++---- .../src/v0/rerun.remote_store.v0.rs | 53 ++++++++----------- 2 files changed, 27 insertions(+), 42 deletions(-) diff --git a/crates/store/re_remote_store_types/proto/rerun/v0/remote_store.proto b/crates/store/re_remote_store_types/proto/rerun/v0/remote_store.proto index 05e42cacd85d..510056293ec7 100644 --- a/crates/store/re_remote_store_types/proto/rerun/v0/remote_store.proto +++ b/crates/store/re_remote_store_types/proto/rerun/v0/remote_store.proto @@ -9,18 +9,17 @@ service StorageNode { rpc Query(QueryRequest) returns (stream QueryResponse) {} rpc FetchRecording(FetchRecordingRequest) returns (stream FetchRecordingResponse) {} rpc GetRecordingMetadata(GetRecordingMetadataRequest) returns (GetRecordingMetadataResponse) {} - // TODO(zehiko) - should this be singular recording registration? Currently we can have 1 rrd => many recordings - rpc RegisterRecordings(RegisterRecordingsRequest) returns (RegisterRecordingsResponse) {} + rpc RegisterRecording(RegisterRecordingRequest) returns (RegisterRecordingResponse) {} } // ---------------- RegisterRecording ------------------ -message RegisterRecordingsRequest { +message RegisterRecordingRequest { // human readable description of the recording string description = 1; // information about recording's backing storage // TODO(zehiko) add separate info about the "source" recording - ObjectStorage obj_storage = 2; + string url = 2; // type of recording RecordingType typ = 3; // (optional) any additional metadata that should be associated with the recording @@ -34,17 +33,12 @@ message RecordingMetadata { bytes payload = 2; } -message ObjectStorage { - string bucket_name = 1; - string url = 2; -} - -message RegisterRecordingsResponse { +message RegisterRecordingResponse { // Note / TODO(zehiko): this implies we read the record (for example go through entire .rrd file // chunk by chunk) and extract the metadata. So we might want to 1/ not do this i.e. // only do it as part of explicit GetMetadata request or 2/ do it if Request has "include_metadata=true" // or 3/ do it always - repeated RecordingMetadata metadata = 2; + RecordingMetadata metadata = 2; } // Server can include details about the error as part of gRPC error (Status) diff --git a/crates/store/re_remote_store_types/src/v0/rerun.remote_store.v0.rs b/crates/store/re_remote_store_types/src/v0/rerun.remote_store.v0.rs index b136d7f6fd47..2208a0b82ef1 100644 --- a/crates/store/re_remote_store_types/src/v0/rerun.remote_store.v0.rs +++ b/crates/store/re_remote_store_types/src/v0/rerun.remote_store.v0.rs @@ -243,14 +243,14 @@ impl ErrorCode { } } #[derive(Clone, PartialEq, ::prost::Message)] -pub struct RegisterRecordingsRequest { +pub struct RegisterRecordingRequest { /// human readable description of the recording #[prost(string, tag = "1")] pub description: ::prost::alloc::string::String, /// information about recording's backing storage /// TODO(zehiko) add separate info about the "source" recording - #[prost(message, optional, tag = "2")] - pub obj_storage: ::core::option::Option, + #[prost(string, tag = "2")] + pub url: ::prost::alloc::string::String, /// type of recording #[prost(enumeration = "RecordingType", tag = "3")] pub typ: i32, @@ -268,20 +268,13 @@ pub struct RecordingMetadata { pub payload: ::prost::alloc::vec::Vec, } #[derive(Clone, PartialEq, ::prost::Message)] -pub struct ObjectStorage { - #[prost(string, tag = "1")] - pub bucket_name: ::prost::alloc::string::String, - #[prost(string, tag = "2")] - pub url: ::prost::alloc::string::String, -} -#[derive(Clone, PartialEq, ::prost::Message)] -pub struct RegisterRecordingsResponse { +pub struct RegisterRecordingResponse { /// Note / TODO(zehiko): this implies we read the record (for example go through entire .rrd file /// chunk by chunk) and extract the metadata. So we might want to 1/ not do this i.e. /// only do it as part of explicit GetMetadata request or 2/ do it if Request has "include_metadata=true" /// or 3/ do it always - #[prost(message, repeated, tag = "2")] - pub metadata: ::prost::alloc::vec::Vec, + #[prost(message, optional, tag = "2")] + pub metadata: ::core::option::Option, } /// Server can include details about the error as part of gRPC error (Status) #[derive(Clone, PartialEq, ::prost::Message)] @@ -577,23 +570,22 @@ pub mod storage_node_client { )); self.inner.unary(req, path, codec).await } - /// TODO(zehiko) - should this be singular recording registration? Currently we can have 1 rrd => many recordings - pub async fn register_recordings( + pub async fn register_recording( &mut self, - request: impl tonic::IntoRequest, - ) -> std::result::Result, tonic::Status> + request: impl tonic::IntoRequest, + ) -> std::result::Result, tonic::Status> { self.inner.ready().await.map_err(|e| { tonic::Status::unknown(format!("Service was not ready: {}", e.into())) })?; let codec = tonic::codec::ProstCodec::default(); let path = http::uri::PathAndQuery::from_static( - "/rerun.remote_store.v0.StorageNode/RegisterRecordings", + "/rerun.remote_store.v0.StorageNode/RegisterRecording", ); let mut req = request.into_request(); req.extensions_mut().insert(GrpcMethod::new( "rerun.remote_store.v0.StorageNode", - "RegisterRecordings", + "RegisterRecording", )); self.inner.unary(req, path, codec).await } @@ -638,11 +630,10 @@ pub mod storage_node_server { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status>; - /// TODO(zehiko) - should this be singular recording registration? Currently we can have 1 rrd => many recordings - async fn register_recordings( + async fn register_recording( &self, - request: tonic::Request, - ) -> std::result::Result, tonic::Status>; + request: tonic::Request, + ) -> std::result::Result, tonic::Status>; } #[derive(Debug)] pub struct StorageNodeServer { @@ -884,22 +875,22 @@ pub mod storage_node_server { }; Box::pin(fut) } - "/rerun.remote_store.v0.StorageNode/RegisterRecordings" => { + "/rerun.remote_store.v0.StorageNode/RegisterRecording" => { #[allow(non_camel_case_types)] - struct RegisterRecordingsSvc(pub Arc); + struct RegisterRecordingSvc(pub Arc); impl - tonic::server::UnaryService - for RegisterRecordingsSvc + tonic::server::UnaryService + for RegisterRecordingSvc { - type Response = super::RegisterRecordingsResponse; + type Response = super::RegisterRecordingResponse; type Future = BoxFuture, tonic::Status>; fn call( &mut self, - request: tonic::Request, + request: tonic::Request, ) -> Self::Future { let inner = Arc::clone(&self.0); let fut = async move { - ::register_recordings(&inner, request).await + ::register_recording(&inner, request).await }; Box::pin(fut) } @@ -910,7 +901,7 @@ pub mod storage_node_server { let max_encoding_message_size = self.max_encoding_message_size; let inner = self.inner.clone(); let fut = async move { - let method = RegisterRecordingsSvc(inner); + let method = RegisterRecordingSvc(inner); let codec = tonic::codec::ProstCodec::default(); let mut grpc = tonic::server::Grpc::new(codec) .apply_compression_config(