From 18ec49843b40e7c59347889343378bf839a10cf4 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 8 Nov 2023 16:06:34 +0000 Subject: [PATCH] Revert "cleanup unused RemoteStorage fields" This reverts commit 8dcd73a588324738ff50cec3d14eb69c0aa239fb. --- compute_tools/src/extension_server.rs | 4 ++- libs/remote_storage/src/lib.rs | 36 ++++++++++++++++++-- libs/remote_storage/tests/test_real_azure.rs | 4 ++- libs/remote_storage/tests/test_real_s3.rs | 4 ++- pageserver/src/config.rs | 8 +++++ pageserver/src/deletion_queue.rs | 8 +++++ pageserver/src/tenant.rs | 4 +++ 7 files changed, 63 insertions(+), 5 deletions(-) diff --git a/compute_tools/src/extension_server.rs b/compute_tools/src/extension_server.rs index 2278509c1f..3d7ed8c360 100644 --- a/compute_tools/src/extension_server.rs +++ b/compute_tools/src/extension_server.rs @@ -78,7 +78,7 @@ use regex::Regex; use remote_storage::*; use serde_json; use std::io::Read; -use std::num::NonZeroUsize; +use std::num::{NonZeroU32, NonZeroUsize}; use std::path::Path; use std::str; use tar::Archive; @@ -281,6 +281,8 @@ pub fn init_remote_storage(remote_ext_config: &str) -> anyhow::Result @@ -382,6 +394,10 @@ pub struct StorageMetadata(HashMap); /// External backup storage configuration, enough for creating a client for that storage. #[derive(Debug, Clone, PartialEq, Eq)] pub struct RemoteStorageConfig { + /// Max allowed number of concurrent sync operations between the API user and the remote storage. + pub max_concurrent_syncs: NonZeroUsize, + /// Max allowed errors before the sync task is considered failed and evicted. + pub max_sync_errors: NonZeroU32, /// The storage connection configuration. pub storage: RemoteStorageKind, } @@ -477,6 +493,18 @@ impl RemoteStorageConfig { let use_azure = container_name.is_some() && container_region.is_some(); + let max_concurrent_syncs = NonZeroUsize::new( + parse_optional_integer("max_concurrent_syncs", toml)? + .unwrap_or(DEFAULT_REMOTE_STORAGE_MAX_CONCURRENT_SYNCS), + ) + .context("Failed to parse 'max_concurrent_syncs' as a positive integer")?; + + let max_sync_errors = NonZeroU32::new( + parse_optional_integer("max_sync_errors", toml)? + .unwrap_or(DEFAULT_REMOTE_STORAGE_MAX_SYNC_ERRORS), + ) + .context("Failed to parse 'max_sync_errors' as a positive integer")?; + let default_concurrency_limit = if use_azure { DEFAULT_REMOTE_STORAGE_AZURE_CONCURRENCY_LIMIT } else { @@ -558,7 +586,11 @@ impl RemoteStorageConfig { } }; - Ok(Some(RemoteStorageConfig { storage })) + Ok(Some(RemoteStorageConfig { + max_concurrent_syncs, + max_sync_errors, + storage, + })) } } diff --git a/libs/remote_storage/tests/test_real_azure.rs b/libs/remote_storage/tests/test_real_azure.rs index 59b92b48fb..0338270aaf 100644 --- a/libs/remote_storage/tests/test_real_azure.rs +++ b/libs/remote_storage/tests/test_real_azure.rs @@ -1,6 +1,6 @@ use std::collections::HashSet; use std::env; -use std::num::NonZeroUsize; +use std::num::{NonZeroU32, NonZeroUsize}; use std::ops::ControlFlow; use std::path::PathBuf; use std::sync::Arc; @@ -469,6 +469,8 @@ fn create_azure_client( let random = rand::thread_rng().gen::(); let remote_storage_config = RemoteStorageConfig { + max_concurrent_syncs: NonZeroUsize::new(100).unwrap(), + max_sync_errors: NonZeroU32::new(5).unwrap(), storage: RemoteStorageKind::AzureContainer(AzureConfig { container_name: remote_storage_azure_container, container_region: remote_storage_azure_region, diff --git a/libs/remote_storage/tests/test_real_s3.rs b/libs/remote_storage/tests/test_real_s3.rs index e2360b7533..7e2aa9f6d7 100644 --- a/libs/remote_storage/tests/test_real_s3.rs +++ b/libs/remote_storage/tests/test_real_s3.rs @@ -1,6 +1,6 @@ use std::collections::HashSet; use std::env; -use std::num::NonZeroUsize; +use std::num::{NonZeroU32, NonZeroUsize}; use std::ops::ControlFlow; use std::path::PathBuf; use std::sync::Arc; @@ -396,6 +396,8 @@ fn create_s3_client( let random = rand::thread_rng().gen::(); let remote_storage_config = RemoteStorageConfig { + max_concurrent_syncs: NonZeroUsize::new(100).unwrap(), + max_sync_errors: NonZeroU32::new(5).unwrap(), storage: RemoteStorageKind::AwsS3(S3Config { bucket_name: remote_storage_s3_bucket, bucket_region: remote_storage_s3_region, diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 4259f91ac6..fe62a7299a 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -1320,6 +1320,12 @@ broker_endpoint = '{broker_endpoint}' assert_eq!( parsed_remote_storage_config, RemoteStorageConfig { + max_concurrent_syncs: NonZeroUsize::new( + remote_storage::DEFAULT_REMOTE_STORAGE_MAX_CONCURRENT_SYNCS + ) + .unwrap(), + max_sync_errors: NonZeroU32::new(remote_storage::DEFAULT_REMOTE_STORAGE_MAX_SYNC_ERRORS) + .unwrap(), storage: RemoteStorageKind::LocalFs(local_storage_path.clone()), }, "Remote storage config should correctly parse the local FS config and fill other storage defaults" @@ -1380,6 +1386,8 @@ broker_endpoint = '{broker_endpoint}' assert_eq!( parsed_remote_storage_config, RemoteStorageConfig { + max_concurrent_syncs, + max_sync_errors, storage: RemoteStorageKind::AwsS3(S3Config { bucket_name: bucket_name.clone(), bucket_region: bucket_region.clone(), diff --git a/pageserver/src/deletion_queue.rs b/pageserver/src/deletion_queue.rs index 4bbae2bf36..22efa23f10 100644 --- a/pageserver/src/deletion_queue.rs +++ b/pageserver/src/deletion_queue.rs @@ -892,6 +892,14 @@ mod test { std::fs::create_dir_all(remote_fs_dir)?; let remote_fs_dir = harness.conf.workdir.join("remote_fs").canonicalize_utf8()?; let storage_config = RemoteStorageConfig { + max_concurrent_syncs: std::num::NonZeroUsize::new( + remote_storage::DEFAULT_REMOTE_STORAGE_MAX_CONCURRENT_SYNCS, + ) + .unwrap(), + max_sync_errors: std::num::NonZeroU32::new( + remote_storage::DEFAULT_REMOTE_STORAGE_MAX_SYNC_ERRORS, + ) + .unwrap(), storage: RemoteStorageKind::LocalFs(remote_fs_dir.clone()), }; let storage = GenericRemoteStorage::from_config(&storage_config).unwrap(); diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 27f7230603..f8895c32dc 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -3728,6 +3728,10 @@ pub(crate) mod harness { let remote_fs_dir = conf.workdir.join("localfs"); std::fs::create_dir_all(&remote_fs_dir).unwrap(); let config = RemoteStorageConfig { + // TODO: why not remote_storage::DEFAULT_REMOTE_STORAGE_MAX_CONCURRENT_SYNCS, + max_concurrent_syncs: std::num::NonZeroUsize::new(2_000_000).unwrap(), + // TODO: why not remote_storage::DEFAULT_REMOTE_STORAGE_MAX_SYNC_ERRORS, + max_sync_errors: std::num::NonZeroU32::new(3_000_000).unwrap(), storage: RemoteStorageKind::LocalFs(remote_fs_dir.clone()), }; let remote_storage = GenericRemoteStorage::from_config(&config).unwrap();