From 85803b250b12677dc21dc376298c469297e6c7d2 Mon Sep 17 00:00:00 2001 From: Patrick Huang <hzxa21@hotmail.com> Date: Tue, 18 Jun 2024 03:46:38 +0000 Subject: [PATCH 1/2] chore: remove the object_store prefix in the object store config name --- src/common/src/config.rs | 162 +++++++++++++++--- src/config/example.toml | 12 +- .../src/object/opendal_engine/fs.rs | 2 +- .../src/object/opendal_engine/opendal_s3.rs | 4 +- src/object_store/src/object/s3.rs | 12 +- .../src/hummock/compactor/compactor_runner.rs | 2 +- 6 files changed, 154 insertions(+), 40 deletions(-) diff --git a/src/common/src/config.rs b/src/common/src/config.rs index d8bb65e90b062..fb9f7c6b5cdf5 100644 --- a/src/common/src/config.rs +++ b/src/common/src/config.rs @@ -1029,8 +1029,12 @@ for_all_params!(define_system_config); /// The subsections `[storage.object_store]`. #[derive(Clone, Debug, Serialize, Deserialize, DefaultFromSerde)] pub struct ObjectStoreConfig { - #[serde(default = "default::object_store_config::object_store_set_atomic_write_dir")] - pub object_store_set_atomic_write_dir: bool, + // alias is for backward compatibility + #[serde( + default = "default::object_store_config::set_atomic_write_dir", + alias = "object_store_set_atomic_write_dir" + )] + pub set_atomic_write_dir: bool, /// Retry and timeout configuration /// Description retry strategy driven by exponential back-off @@ -1045,25 +1049,36 @@ pub struct ObjectStoreConfig { impl ObjectStoreConfig { pub fn set_atomic_write_dir(&mut self) { - self.object_store_set_atomic_write_dir = true; + self.set_atomic_write_dir = true; } } /// The subsections `[storage.object_store.s3]`. #[derive(Clone, Debug, Serialize, Deserialize, DefaultFromSerde)] pub struct S3ObjectStoreConfig { - #[serde(default = "default::object_store_config::s3::object_store_keepalive_ms")] - pub object_store_keepalive_ms: Option<u64>, - #[serde(default = "default::object_store_config::s3::object_store_recv_buffer_size")] - pub object_store_recv_buffer_size: Option<usize>, - #[serde(default = "default::object_store_config::s3::object_store_send_buffer_size")] - pub object_store_send_buffer_size: Option<usize>, - #[serde(default = "default::object_store_config::s3::object_store_nodelay")] - pub object_store_nodelay: Option<bool>, - /// For backwards compatibility, users should use `S3ObjectStoreDeveloperConfig` instead. + // alias is for backward compatibility + #[serde( + default = "default::object_store_config::s3::keepalive_ms", + alias = "object_store_keepalive_ms" + )] + pub keepalive_ms: Option<u64>, + #[serde( + default = "default::object_store_config::s3::recv_buffer_size", + alias = "object_store_recv_buffer_size" + )] + pub recv_buffer_size: Option<usize>, + #[serde( + default = "default::object_store_config::s3::send_buffer_size", + alias = "object_store_send_buffer_size" + )] + pub send_buffer_size: Option<usize>, #[serde( - default = "default::object_store_config::s3::developer::object_store_retry_unknown_service_error" + default = "default::object_store_config::s3::nodelay", + alias = "object_store_nodelay" )] + pub nodelay: Option<bool>, + /// For backwards compatibility, users should use `S3ObjectStoreDeveloperConfig` instead. + #[serde(default = "default::object_store_config::s3::developer::retry_unknown_service_error")] pub retry_unknown_service_error: bool, #[serde(default = "default::object_store_config::s3::identity_resolution_timeout_s")] pub identity_resolution_timeout_s: u64, @@ -1076,15 +1091,17 @@ pub struct S3ObjectStoreConfig { pub struct S3ObjectStoreDeveloperConfig { /// Whether to retry s3 sdk error from which no error metadata is provided. #[serde( - default = "default::object_store_config::s3::developer::object_store_retry_unknown_service_error" + default = "default::object_store_config::s3::developer::retry_unknown_service_error", + alias = "object_store_retry_unknown_service_error" )] - pub object_store_retry_unknown_service_error: bool, + pub retry_unknown_service_error: bool, /// An array of error codes that should be retried. /// e.g. `["SlowDown", "TooManyRequests"]` #[serde( - default = "default::object_store_config::s3::developer::object_store_retryable_service_error_codes" + default = "default::object_store_config::s3::developer::retryable_service_error_codes", + alias = "object_store_retryable_service_error_codes" )] - pub object_store_retryable_service_error_codes: Vec<String>, + pub retryable_service_error_codes: Vec<String>, #[serde(default = "default::object_store_config::s3::developer::use_opendal")] pub use_opendal: bool, @@ -1904,7 +1921,7 @@ pub mod default { const DEFAULT_REQ_BACKOFF_MAX_DELAY_MS: u64 = 10 * 1000; // 10s const DEFAULT_REQ_MAX_RETRY_ATTEMPTS: usize = 3; - pub fn object_store_set_atomic_write_dir() -> bool { + pub fn set_atomic_write_dir() -> bool { false } @@ -1992,19 +2009,19 @@ pub mod default { const DEFAULT_KEEPALIVE_MS: u64 = 600 * 1000; // 10min - pub fn object_store_keepalive_ms() -> Option<u64> { + pub fn keepalive_ms() -> Option<u64> { Some(DEFAULT_KEEPALIVE_MS) // 10min } - pub fn object_store_recv_buffer_size() -> Option<usize> { + pub fn recv_buffer_size() -> Option<usize> { Some(1 << 21) // 2m } - pub fn object_store_send_buffer_size() -> Option<usize> { + pub fn send_buffer_size() -> Option<usize> { None } - pub fn object_store_nodelay() -> Option<bool> { + pub fn nodelay() -> Option<bool> { Some(true) } @@ -2017,11 +2034,11 @@ pub mod default { const RW_USE_OPENDAL_FOR_S3: &str = "RW_USE_OPENDAL_FOR_S3"; - pub fn object_store_retry_unknown_service_error() -> bool { + pub fn retry_unknown_service_error() -> bool { false } - pub fn object_store_retryable_service_error_codes() -> Vec<String> { + pub fn retryable_service_error_codes() -> Vec<String> { vec!["SlowDown".into(), "TooManyRequests".into()] } @@ -2336,4 +2353,101 @@ mod tests { } } } + + #[test] + fn test_object_store_configs_backward_compatibility() { + // Define configs with the old name and make sure it still works + { + let config: RwConfig = toml::from_str( + r#" + [storage.object_store] + object_store_set_atomic_write_dir = true + + [storage.object_store.s3] + object_store_keepalive_ms = 1 + object_store_send_buffer_size = 1 + object_store_recv_buffer_size = 1 + object_store_nodelay = false + + [storage.object_store.s3.developer] + object_store_retry_unknown_service_error = true + object_store_retryable_service_error_codes = ['dummy'] + + + "#, + ) + .unwrap(); + + assert_eq!(config.storage.object_store.set_atomic_write_dir, true); + assert_eq!(config.storage.object_store.s3.keepalive_ms, Some(1)); + assert_eq!(config.storage.object_store.s3.send_buffer_size, Some(1)); + assert_eq!(config.storage.object_store.s3.recv_buffer_size, Some(1)); + assert_eq!(config.storage.object_store.s3.nodelay, Some(false)); + assert_eq!( + config + .storage + .object_store + .s3 + .developer + .retry_unknown_service_error, + true + ); + assert_eq!( + config + .storage + .object_store + .s3 + .developer + .retryable_service_error_codes, + vec!["dummy".to_string()] + ); + } + + // Define configs with the new name and make sure it works + { + let config: RwConfig = toml::from_str( + r#" + [storage.object_store] + set_atomic_write_dir = true + + [storage.object_store.s3] + keepalive_ms = 1 + send_buffer_size = 1 + recv_buffer_size = 1 + nodelay = false + + [storage.object_store.s3.developer] + retry_unknown_service_error = true + retryable_service_error_codes = ['dummy'] + + + "#, + ) + .unwrap(); + + assert_eq!(config.storage.object_store.set_atomic_write_dir, true); + assert_eq!(config.storage.object_store.s3.keepalive_ms, Some(1)); + assert_eq!(config.storage.object_store.s3.send_buffer_size, Some(1)); + assert_eq!(config.storage.object_store.s3.recv_buffer_size, Some(1)); + assert_eq!(config.storage.object_store.s3.nodelay, Some(false)); + assert_eq!( + config + .storage + .object_store + .s3 + .developer + .retry_unknown_service_error, + true + ); + assert_eq!( + config + .storage + .object_store + .s3 + .developer + .retryable_service_error_codes, + vec!["dummy".to_string()] + ); + } + } } diff --git a/src/config/example.toml b/src/config/example.toml index b35590c85059b..27bbea13ade15 100644 --- a/src/config/example.toml +++ b/src/config/example.toml @@ -190,7 +190,7 @@ recent_filter_layers = 6 recent_filter_rotate_interval_ms = 10000 [storage.object_store] -object_store_set_atomic_write_dir = false +set_atomic_write_dir = false [storage.object_store.retry] req_backoff_interval_ms = 1000 @@ -214,15 +214,15 @@ list_attempt_timeout_ms = 600000 list_retry_attempts = 3 [storage.object_store.s3] -object_store_keepalive_ms = 600000 -object_store_recv_buffer_size = 2097152 -object_store_nodelay = true +keepalive_ms = 600000 +recv_buffer_size = 2097152 +nodelay = true retry_unknown_service_error = false identity_resolution_timeout_s = 5 [storage.object_store.s3.developer] -object_store_retry_unknown_service_error = false -object_store_retryable_service_error_codes = ["SlowDown", "TooManyRequests"] +retry_unknown_service_error = false +retryable_service_error_codes = ["SlowDown", "TooManyRequests"] use_opendal = false [system] diff --git a/src/object_store/src/object/opendal_engine/fs.rs b/src/object_store/src/object/opendal_engine/fs.rs index 4f2715d6ccfb8..ecb1131f0def8 100644 --- a/src/object_store/src/object/opendal_engine/fs.rs +++ b/src/object_store/src/object/opendal_engine/fs.rs @@ -29,7 +29,7 @@ impl OpendalObjectStore { // Create fs backend builder. let mut builder = Fs::default(); builder.root(&root); - if config.object_store_set_atomic_write_dir { + if config.set_atomic_write_dir { let atomic_write_dir = format!("{}/{}", root, ATOMIC_WRITE_DIR); builder.atomic_write_dir(&atomic_write_dir); } diff --git a/src/object_store/src/object/opendal_engine/opendal_s3.rs b/src/object_store/src/object/opendal_engine/opendal_s3.rs index 7a51cbb36955f..e86a209f4f3fa 100644 --- a/src/object_store/src/object/opendal_engine/opendal_s3.rs +++ b/src/object_store/src/object/opendal_engine/opendal_s3.rs @@ -95,11 +95,11 @@ impl OpendalObjectStore { pub fn new_http_client(config: &ObjectStoreConfig) -> ObjectResult<HttpClient> { let mut client_builder = reqwest::ClientBuilder::new(); - if let Some(keepalive_ms) = config.s3.object_store_keepalive_ms.as_ref() { + if let Some(keepalive_ms) = config.s3.keepalive_ms.as_ref() { client_builder = client_builder.tcp_keepalive(Duration::from_millis(*keepalive_ms)); } - if let Some(nodelay) = config.s3.object_store_nodelay.as_ref() { + if let Some(nodelay) = config.s3.nodelay.as_ref() { client_builder = client_builder.tcp_nodelay(*nodelay); } diff --git a/src/object_store/src/object/s3.rs b/src/object_store/src/object/s3.rs index f1f569cb7d36e..3ed5fc01ba40c 100644 --- a/src/object_store/src/object/s3.rs +++ b/src/object_store/src/object/s3.rs @@ -618,19 +618,19 @@ impl S3ObjectStore { let mut http = hyper::client::HttpConnector::new(); // connection config - if let Some(keepalive_ms) = config.s3.object_store_keepalive_ms.as_ref() { + if let Some(keepalive_ms) = config.s3.keepalive_ms.as_ref() { http.set_keepalive(Some(Duration::from_millis(*keepalive_ms))); } - if let Some(nodelay) = config.s3.object_store_nodelay.as_ref() { + if let Some(nodelay) = config.s3.nodelay.as_ref() { http.set_nodelay(*nodelay); } - if let Some(recv_buffer_size) = config.s3.object_store_recv_buffer_size.as_ref() { + if let Some(recv_buffer_size) = config.s3.recv_buffer_size.as_ref() { http.set_recv_buffer_size(Some(*recv_buffer_size)); } - if let Some(send_buffer_size) = config.s3.object_store_send_buffer_size.as_ref() { + if let Some(send_buffer_size) = config.s3.send_buffer_size.as_ref() { http.set_send_buffer_size(Some(*send_buffer_size)); } @@ -1043,7 +1043,7 @@ where Some(SdkError::ServiceError(e)) => { let retry = match e.err().code() { None => { - if config.s3.developer.object_store_retry_unknown_service_error + if config.s3.developer.retry_unknown_service_error || config.s3.retry_unknown_service_error { tracing::warn!(target: "unknown_service_error", "{e:?} occurs, retry S3 get_object request."); @@ -1056,7 +1056,7 @@ where if config .s3 .developer - .object_store_retryable_service_error_codes + .retryable_service_error_codes .iter() .any(|s| s.as_str().eq_ignore_ascii_case(code)) { diff --git a/src/storage/src/hummock/compactor/compactor_runner.rs b/src/storage/src/hummock/compactor/compactor_runner.rs index 41a55518158de..0336f6f542fa4 100644 --- a/src/storage/src/hummock/compactor/compactor_runner.rs +++ b/src/storage/src/hummock/compactor/compactor_runner.rs @@ -380,7 +380,7 @@ pub async fn compact( .storage_opts .object_store_config .s3 - .object_store_recv_buffer_size + .recv_buffer_size .unwrap_or(6 * 1024 * 1024) as u64, capacity as u64, ) * compact_task.splits.len() as u64; From ebd6f6ab4a7807cd5a51fc0da00e0eacfaf14dd5 Mon Sep 17 00:00:00 2001 From: Patrick Huang <hzxa21@hotmail.com> Date: Tue, 18 Jun 2024 05:18:18 +0000 Subject: [PATCH 2/2] fix check --- src/common/src/config.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/common/src/config.rs b/src/common/src/config.rs index fb9f7c6b5cdf5..0dc6b48d2d8da 100644 --- a/src/common/src/config.rs +++ b/src/common/src/config.rs @@ -2378,19 +2378,18 @@ mod tests { ) .unwrap(); - assert_eq!(config.storage.object_store.set_atomic_write_dir, true); + assert!(config.storage.object_store.set_atomic_write_dir); assert_eq!(config.storage.object_store.s3.keepalive_ms, Some(1)); assert_eq!(config.storage.object_store.s3.send_buffer_size, Some(1)); assert_eq!(config.storage.object_store.s3.recv_buffer_size, Some(1)); assert_eq!(config.storage.object_store.s3.nodelay, Some(false)); - assert_eq!( + assert!( config .storage .object_store .s3 .developer - .retry_unknown_service_error, - true + .retry_unknown_service_error ); assert_eq!( config @@ -2425,19 +2424,18 @@ mod tests { ) .unwrap(); - assert_eq!(config.storage.object_store.set_atomic_write_dir, true); + assert!(config.storage.object_store.set_atomic_write_dir); assert_eq!(config.storage.object_store.s3.keepalive_ms, Some(1)); assert_eq!(config.storage.object_store.s3.send_buffer_size, Some(1)); assert_eq!(config.storage.object_store.s3.recv_buffer_size, Some(1)); assert_eq!(config.storage.object_store.s3.nodelay, Some(false)); - assert_eq!( + assert!( config .storage .object_store .s3 .developer - .retry_unknown_service_error, - true + .retry_unknown_service_error ); assert_eq!( config