From 74933a77fa804f482bb34be5dd321b214c50c4a5 Mon Sep 17 00:00:00 2001 From: "Chanyub.Park" Date: Thu, 29 Aug 2024 18:53:53 +0900 Subject: [PATCH 1/6] Add no_hardlinks option to LocalConfig and fix error handling --- object_store_factory/src/local.rs | 78 ++++++++++++++++++++++++++++++- src/config/schema.rs | 2 + src/context/delta.rs | 1 + src/object_store/wrapped.rs | 9 +++- 4 files changed, 87 insertions(+), 3 deletions(-) diff --git a/object_store_factory/src/local.rs b/object_store_factory/src/local.rs index acfa6db1..6ca3d782 100644 --- a/object_store_factory/src/local.rs +++ b/object_store_factory/src/local.rs @@ -6,6 +6,12 @@ use std::sync::Arc; #[derive(Deserialize, Debug, PartialEq, Eq, Clone)] pub struct LocalConfig { pub data_dir: String, + #[serde(default = "default_false")] + pub no_hardlinks: bool, +} + +fn default_false() -> bool { + false } impl LocalConfig { @@ -13,13 +19,20 @@ impl LocalConfig { map: &HashMap, ) -> Result { Ok(Self { - data_dir: map.get("data_dir").unwrap().clone(), + data_dir: map.get("data_dir") + .ok_or_else(|| object_store::Error::Generic { + store: "local", + source: "Missing data_dir".into(), + })? + .clone(), + no_hardlinks: map.get("no_hardlinks").map(|s| s == "true").unwrap_or(false), }) } pub fn to_hashmap(&self) -> HashMap { let mut map = HashMap::new(); map.insert("data_dir".to_string(), self.data_dir.clone()); + map.insert("no_hardlinks".to_string(), self.no_hardlinks.to_string()); map } @@ -44,6 +57,31 @@ mod tests { let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); + assert_eq!(config.no_hardlinks, false); // Default value + } + + #[test] + fn test_config_from_hashmap_with_no_hardlinks() { + let mut map = HashMap::new(); + map.insert("data_dir".to_string(), "/tmp/data".to_string()); + map.insert("no_hardlinks".to_string(), "true".to_string()); + + let config = LocalConfig::from_hashmap(&map) + .expect("Failed to create config from hashmap"); + assert_eq!(config.data_dir, "/tmp/data".to_string()); + assert_eq!(config.no_hardlinks, true); + } + + #[test] + fn test_config_from_hashmap_with_no_hardlinks_false() { + let mut map = HashMap::new(); + map.insert("data_dir".to_string(), "/tmp/data".to_string()); + map.insert("no_hardlinks".to_string(), "false".to_string()); + + let config = LocalConfig::from_hashmap(&map) + .expect("Failed to create config from hashmap"); + assert_eq!(config.data_dir, "/tmp/data".to_string()); + assert_eq!(config.no_hardlinks, false); } #[test] @@ -64,6 +102,7 @@ mod tests { let result = LocalConfig { data_dir: data_dir.to_string(), + no_hardlinks: false, } .build_local_storage(); assert!(result.is_ok(), "Expected Ok, got Err: {:?}", result); @@ -73,6 +112,7 @@ mod tests { fn test_build_local_storage_with_invalid_path() { let result = LocalConfig { data_dir: "".to_string(), + no_hardlinks: false, } .build_local_storage(); assert!(result.is_err(), "Expected Err due to invalid path, got Ok"); @@ -82,10 +122,44 @@ mod tests { fn test_to_hashmap() { let local_config = LocalConfig { data_dir: "path/to/data".to_string(), + no_hardlinks: true, }; let hashmap = local_config.to_hashmap(); assert_eq!(hashmap.get("data_dir"), Some(&"path/to/data".to_string())); + assert_eq!(hashmap.get("no_hardlinks"), Some(&"true".to_string())); } -} + + #[test] + fn test_default_false() { + assert_eq!(default_false(), false); + } + + #[test] + fn test_deserialize_with_default() { + let json = r#" + { + "data_dir": "/tmp/data" + } + "#; + + let config: LocalConfig = serde_json::from_str(json).unwrap(); + assert_eq!(config.data_dir, "/tmp/data"); + assert_eq!(config.no_hardlinks, false); + } + + #[test] + fn test_deserialize_with_no_hardlinks() { + let json = r#" + { + "data_dir": "/tmp/data", + "no_hardlinks": true + } + "#; + + let config: LocalConfig = serde_json::from_str(json).unwrap(); + assert_eq!(config.data_dir, "/tmp/data"); + assert_eq!(config.no_hardlinks, true); + } +} \ No newline at end of file diff --git a/src/config/schema.rs b/src/config/schema.rs index 14483b86..d7bc35b4 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -637,6 +637,7 @@ cache_control = "private, max-age=86400" SeafowlConfig { object_store: Some(ObjectStoreConfig::Local(LocalConfig { data_dir: "./seafowl-data".to_string(), + no_hardlinks: false, })), catalog: Some(Catalog::Postgres(Postgres { dsn: "postgresql://user:pass@localhost:5432/somedb".to_string(), @@ -732,6 +733,7 @@ cache_control = "private, max-age=86400" SeafowlConfig { object_store: Some(ObjectStoreConfig::Local(LocalConfig { data_dir: "some_other_path".to_string(), + no_hardlinks: false, })), catalog: Some(Catalog::Sqlite(Sqlite { dsn: "sqlite://file.sqlite".to_string(), diff --git a/src/context/delta.rs b/src/context/delta.rs index 02c88e81..a97e5c87 100644 --- a/src/context/delta.rs +++ b/src/context/delta.rs @@ -528,6 +528,7 @@ mod tests { Arc::new(LocalFileSystem::new_with_prefix(tmp_dir.path()).unwrap()), ObjectStoreConfig::Local(LocalConfig { data_dir: tmp_dir.path().to_string_lossy().to_string(), + no_hardlinks: false, }), ), Some(tmp_dir), diff --git a/src/object_store/wrapped.rs b/src/object_store/wrapped.rs index 84dcbe1a..35a0f624 100644 --- a/src/object_store/wrapped.rs +++ b/src/object_store/wrapped.rs @@ -17,6 +17,7 @@ use url::Url; use object_store_factory::aws::S3Config; use object_store_factory::google::GCSConfig; use object_store_factory::ObjectStoreConfig; +use object_store_factory::local::LocalConfig; // Wrapper around the object_store crate that holds on to the original config // in order to provide a more efficient "upload" for the local object store @@ -239,6 +240,9 @@ impl ObjectStore for InternalObjectStore { /// /// Will return an error if the destination already has an object. async fn copy_if_not_exists(&self, from: &Path, to: &Path) -> Result<()> { + if let ObjectStoreConfig::Local(LocalConfig { no_hardlinks: true, .. }) = self.config { + return self.inner.copy(from, to).await; + } self.inner.copy_if_not_exists(from, to).await } @@ -254,6 +258,9 @@ impl ObjectStore for InternalObjectStore { // this with a lock too, so look into using that down the line instead. return self.inner.rename(from, to).await; } + if let ObjectStoreConfig::Local(LocalConfig { no_hardlinks: true, .. }) = self.config { + return self.inner.rename(from, to).await; + } self.inner.rename_if_not_exists(from, to).await } } @@ -264,7 +271,7 @@ mod tests { use crate::object_store::wrapped::InternalObjectStore; use datafusion::common::Result; use rstest::rstest; - + use object_store_factory::aws::S3Config; use object_store_factory::ObjectStoreConfig; From 4a63cb485175d917f917519b6ae4c7fb3845ae1b Mon Sep 17 00:00:00 2001 From: "Chanyub.Park" Date: Mon, 2 Sep 2024 17:37:11 +0900 Subject: [PATCH 2/6] no_hardlinks to disable_hardlinks --- object_store_factory/src/local.rs | 36 +++++++++++++++---------------- src/config/schema.rs | 4 ++-- src/context/delta.rs | 2 +- src/object_store/wrapped.rs | 4 ++-- 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/object_store_factory/src/local.rs b/object_store_factory/src/local.rs index 6ca3d782..9d107a42 100644 --- a/object_store_factory/src/local.rs +++ b/object_store_factory/src/local.rs @@ -7,7 +7,7 @@ use std::sync::Arc; pub struct LocalConfig { pub data_dir: String, #[serde(default = "default_false")] - pub no_hardlinks: bool, + pub disable_hardlinks: bool, } fn default_false() -> bool { @@ -25,14 +25,14 @@ impl LocalConfig { source: "Missing data_dir".into(), })? .clone(), - no_hardlinks: map.get("no_hardlinks").map(|s| s == "true").unwrap_or(false), + disable_hardlinks: map.get("disable_hardlinks").map(|s| s == "true").unwrap_or(false), }) } pub fn to_hashmap(&self) -> HashMap { let mut map = HashMap::new(); map.insert("data_dir".to_string(), self.data_dir.clone()); - map.insert("no_hardlinks".to_string(), self.no_hardlinks.to_string()); + map.insert("disable_hardlinks".to_string(), self.disable_hardlinks.to_string()); map } @@ -57,31 +57,31 @@ mod tests { let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.no_hardlinks, false); // Default value + assert_eq!(config.disable_hardlinks, false); // Default value } #[test] - fn test_config_from_hashmap_with_no_hardlinks() { + fn test_config_from_hashmap_with_disable_hardlinks() { let mut map = HashMap::new(); map.insert("data_dir".to_string(), "/tmp/data".to_string()); - map.insert("no_hardlinks".to_string(), "true".to_string()); + map.insert("disable_hardlinks".to_string(), "true".to_string()); let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.no_hardlinks, true); + assert_eq!(config.disable_hardlinks, true); } #[test] - fn test_config_from_hashmap_with_no_hardlinks_false() { + fn test_config_from_hashmap_with_disable_hardlinks_false() { let mut map = HashMap::new(); map.insert("data_dir".to_string(), "/tmp/data".to_string()); - map.insert("no_hardlinks".to_string(), "false".to_string()); + map.insert("disable_hardlinks".to_string(), "false".to_string()); let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.no_hardlinks, false); + assert_eq!(config.disable_hardlinks, false); } #[test] @@ -102,7 +102,7 @@ mod tests { let result = LocalConfig { data_dir: data_dir.to_string(), - no_hardlinks: false, + disable_hardlinks: false, } .build_local_storage(); assert!(result.is_ok(), "Expected Ok, got Err: {:?}", result); @@ -112,7 +112,7 @@ mod tests { fn test_build_local_storage_with_invalid_path() { let result = LocalConfig { data_dir: "".to_string(), - no_hardlinks: false, + disable_hardlinks: false, } .build_local_storage(); assert!(result.is_err(), "Expected Err due to invalid path, got Ok"); @@ -122,13 +122,13 @@ mod tests { fn test_to_hashmap() { let local_config = LocalConfig { data_dir: "path/to/data".to_string(), - no_hardlinks: true, + disable_hardlinks: true, }; let hashmap = local_config.to_hashmap(); assert_eq!(hashmap.get("data_dir"), Some(&"path/to/data".to_string())); - assert_eq!(hashmap.get("no_hardlinks"), Some(&"true".to_string())); + assert_eq!(hashmap.get("disable_hardlinks"), Some(&"true".to_string())); } #[test] @@ -146,20 +146,20 @@ mod tests { let config: LocalConfig = serde_json::from_str(json).unwrap(); assert_eq!(config.data_dir, "/tmp/data"); - assert_eq!(config.no_hardlinks, false); + assert_eq!(config.disable_hardlinks, false); } #[test] - fn test_deserialize_with_no_hardlinks() { + fn test_deserialize_with_disable_hardlinks() { let json = r#" { "data_dir": "/tmp/data", - "no_hardlinks": true + "disable_hardlinks": true } "#; let config: LocalConfig = serde_json::from_str(json).unwrap(); assert_eq!(config.data_dir, "/tmp/data"); - assert_eq!(config.no_hardlinks, true); + assert_eq!(config.disable_hardlinks, true); } } \ No newline at end of file diff --git a/src/config/schema.rs b/src/config/schema.rs index d7bc35b4..fd271939 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -637,7 +637,7 @@ cache_control = "private, max-age=86400" SeafowlConfig { object_store: Some(ObjectStoreConfig::Local(LocalConfig { data_dir: "./seafowl-data".to_string(), - no_hardlinks: false, + disable_hardlinks: false, })), catalog: Some(Catalog::Postgres(Postgres { dsn: "postgresql://user:pass@localhost:5432/somedb".to_string(), @@ -733,7 +733,7 @@ cache_control = "private, max-age=86400" SeafowlConfig { object_store: Some(ObjectStoreConfig::Local(LocalConfig { data_dir: "some_other_path".to_string(), - no_hardlinks: false, + disable_hardlinks: false, })), catalog: Some(Catalog::Sqlite(Sqlite { dsn: "sqlite://file.sqlite".to_string(), diff --git a/src/context/delta.rs b/src/context/delta.rs index a97e5c87..375480c2 100644 --- a/src/context/delta.rs +++ b/src/context/delta.rs @@ -528,7 +528,7 @@ mod tests { Arc::new(LocalFileSystem::new_with_prefix(tmp_dir.path()).unwrap()), ObjectStoreConfig::Local(LocalConfig { data_dir: tmp_dir.path().to_string_lossy().to_string(), - no_hardlinks: false, + disable_hardlinks: false, }), ), Some(tmp_dir), diff --git a/src/object_store/wrapped.rs b/src/object_store/wrapped.rs index 35a0f624..04f1eba2 100644 --- a/src/object_store/wrapped.rs +++ b/src/object_store/wrapped.rs @@ -240,7 +240,7 @@ impl ObjectStore for InternalObjectStore { /// /// Will return an error if the destination already has an object. async fn copy_if_not_exists(&self, from: &Path, to: &Path) -> Result<()> { - if let ObjectStoreConfig::Local(LocalConfig { no_hardlinks: true, .. }) = self.config { + if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { return self.inner.copy(from, to).await; } self.inner.copy_if_not_exists(from, to).await @@ -258,7 +258,7 @@ impl ObjectStore for InternalObjectStore { // this with a lock too, so look into using that down the line instead. return self.inner.rename(from, to).await; } - if let ObjectStoreConfig::Local(LocalConfig { no_hardlinks: true, .. }) = self.config { + if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { return self.inner.rename(from, to).await; } self.inner.rename_if_not_exists(from, to).await From 25b4822586c43fbd8b67f43beab7386e50e970c0 Mon Sep 17 00:00:00 2001 From: "Chanyub.Park" Date: Sat, 31 Aug 2024 15:14:05 +0000 Subject: [PATCH 3/6] add print config for debug --- src/main.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 90c96fda..46eab5f4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,7 +29,7 @@ use seafowl::{ use tokio::time::{interval, Duration}; use tracing::level_filters::LevelFilter; -use tracing::{error, info, subscriber, warn}; +use tracing::{error, info, debug, subscriber, warn}; use tracing_log::LogTracer; use tracing_subscriber::filter::EnvFilter; @@ -157,6 +157,9 @@ async fn main() { config }; + debug!("Input configuration: {:?}", args); + debug!("Starting configuration: {:?}", config); + if !args.cli && let Some(ref metrics) = config.misc.metrics { From 46fcdd6720f66fcfaa141ad3cae693eec816e51c Mon Sep 17 00:00:00 2001 From: "Chanyub.Park" Date: Mon, 2 Sep 2024 18:18:14 +0900 Subject: [PATCH 4/6] no_hardlinks to disable_hardlinks --- object_store_factory/src/local.rs | 36 +++++++++++++++---------------- src/config/schema.rs | 4 ++-- src/context/delta.rs | 2 +- src/object_store/wrapped.rs | 11 ++++++---- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/object_store_factory/src/local.rs b/object_store_factory/src/local.rs index 6ca3d782..9d107a42 100644 --- a/object_store_factory/src/local.rs +++ b/object_store_factory/src/local.rs @@ -7,7 +7,7 @@ use std::sync::Arc; pub struct LocalConfig { pub data_dir: String, #[serde(default = "default_false")] - pub no_hardlinks: bool, + pub disable_hardlinks: bool, } fn default_false() -> bool { @@ -25,14 +25,14 @@ impl LocalConfig { source: "Missing data_dir".into(), })? .clone(), - no_hardlinks: map.get("no_hardlinks").map(|s| s == "true").unwrap_or(false), + disable_hardlinks: map.get("disable_hardlinks").map(|s| s == "true").unwrap_or(false), }) } pub fn to_hashmap(&self) -> HashMap { let mut map = HashMap::new(); map.insert("data_dir".to_string(), self.data_dir.clone()); - map.insert("no_hardlinks".to_string(), self.no_hardlinks.to_string()); + map.insert("disable_hardlinks".to_string(), self.disable_hardlinks.to_string()); map } @@ -57,31 +57,31 @@ mod tests { let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.no_hardlinks, false); // Default value + assert_eq!(config.disable_hardlinks, false); // Default value } #[test] - fn test_config_from_hashmap_with_no_hardlinks() { + fn test_config_from_hashmap_with_disable_hardlinks() { let mut map = HashMap::new(); map.insert("data_dir".to_string(), "/tmp/data".to_string()); - map.insert("no_hardlinks".to_string(), "true".to_string()); + map.insert("disable_hardlinks".to_string(), "true".to_string()); let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.no_hardlinks, true); + assert_eq!(config.disable_hardlinks, true); } #[test] - fn test_config_from_hashmap_with_no_hardlinks_false() { + fn test_config_from_hashmap_with_disable_hardlinks_false() { let mut map = HashMap::new(); map.insert("data_dir".to_string(), "/tmp/data".to_string()); - map.insert("no_hardlinks".to_string(), "false".to_string()); + map.insert("disable_hardlinks".to_string(), "false".to_string()); let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.no_hardlinks, false); + assert_eq!(config.disable_hardlinks, false); } #[test] @@ -102,7 +102,7 @@ mod tests { let result = LocalConfig { data_dir: data_dir.to_string(), - no_hardlinks: false, + disable_hardlinks: false, } .build_local_storage(); assert!(result.is_ok(), "Expected Ok, got Err: {:?}", result); @@ -112,7 +112,7 @@ mod tests { fn test_build_local_storage_with_invalid_path() { let result = LocalConfig { data_dir: "".to_string(), - no_hardlinks: false, + disable_hardlinks: false, } .build_local_storage(); assert!(result.is_err(), "Expected Err due to invalid path, got Ok"); @@ -122,13 +122,13 @@ mod tests { fn test_to_hashmap() { let local_config = LocalConfig { data_dir: "path/to/data".to_string(), - no_hardlinks: true, + disable_hardlinks: true, }; let hashmap = local_config.to_hashmap(); assert_eq!(hashmap.get("data_dir"), Some(&"path/to/data".to_string())); - assert_eq!(hashmap.get("no_hardlinks"), Some(&"true".to_string())); + assert_eq!(hashmap.get("disable_hardlinks"), Some(&"true".to_string())); } #[test] @@ -146,20 +146,20 @@ mod tests { let config: LocalConfig = serde_json::from_str(json).unwrap(); assert_eq!(config.data_dir, "/tmp/data"); - assert_eq!(config.no_hardlinks, false); + assert_eq!(config.disable_hardlinks, false); } #[test] - fn test_deserialize_with_no_hardlinks() { + fn test_deserialize_with_disable_hardlinks() { let json = r#" { "data_dir": "/tmp/data", - "no_hardlinks": true + "disable_hardlinks": true } "#; let config: LocalConfig = serde_json::from_str(json).unwrap(); assert_eq!(config.data_dir, "/tmp/data"); - assert_eq!(config.no_hardlinks, true); + assert_eq!(config.disable_hardlinks, true); } } \ No newline at end of file diff --git a/src/config/schema.rs b/src/config/schema.rs index d7bc35b4..fd271939 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -637,7 +637,7 @@ cache_control = "private, max-age=86400" SeafowlConfig { object_store: Some(ObjectStoreConfig::Local(LocalConfig { data_dir: "./seafowl-data".to_string(), - no_hardlinks: false, + disable_hardlinks: false, })), catalog: Some(Catalog::Postgres(Postgres { dsn: "postgresql://user:pass@localhost:5432/somedb".to_string(), @@ -733,7 +733,7 @@ cache_control = "private, max-age=86400" SeafowlConfig { object_store: Some(ObjectStoreConfig::Local(LocalConfig { data_dir: "some_other_path".to_string(), - no_hardlinks: false, + disable_hardlinks: false, })), catalog: Some(Catalog::Sqlite(Sqlite { dsn: "sqlite://file.sqlite".to_string(), diff --git a/src/context/delta.rs b/src/context/delta.rs index a97e5c87..375480c2 100644 --- a/src/context/delta.rs +++ b/src/context/delta.rs @@ -528,7 +528,7 @@ mod tests { Arc::new(LocalFileSystem::new_with_prefix(tmp_dir.path()).unwrap()), ObjectStoreConfig::Local(LocalConfig { data_dir: tmp_dir.path().to_string_lossy().to_string(), - no_hardlinks: false, + disable_hardlinks: false, }), ), Some(tmp_dir), diff --git a/src/object_store/wrapped.rs b/src/object_store/wrapped.rs index 35a0f624..ad4446db 100644 --- a/src/object_store/wrapped.rs +++ b/src/object_store/wrapped.rs @@ -152,6 +152,9 @@ impl ObjectStore for InternalObjectStore { payload: PutPayload, opts: PutOptions, ) -> Result { + if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { + return self.inner.put_opts(location, payload, PutOptions{mode: object_store::PutMode::Overwrite, ..opts}).await + }; self.inner.put_opts(location, payload, opts).await } @@ -240,9 +243,9 @@ impl ObjectStore for InternalObjectStore { /// /// Will return an error if the destination already has an object. async fn copy_if_not_exists(&self, from: &Path, to: &Path) -> Result<()> { - if let ObjectStoreConfig::Local(LocalConfig { no_hardlinks: true, .. }) = self.config { - return self.inner.copy(from, to).await; - } + if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { + return self.inner.copy(from, to).await; + } self.inner.copy_if_not_exists(from, to).await } @@ -258,7 +261,7 @@ impl ObjectStore for InternalObjectStore { // this with a lock too, so look into using that down the line instead. return self.inner.rename(from, to).await; } - if let ObjectStoreConfig::Local(LocalConfig { no_hardlinks: true, .. }) = self.config { + if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { return self.inner.rename(from, to).await; } self.inner.rename_if_not_exists(from, to).await From 20fa9ccbcbb9fb16450cd9d7cc432e81eb7a798f Mon Sep 17 00:00:00 2001 From: Artjoms Iskovs Date: Tue, 3 Sep 2024 09:50:03 +0100 Subject: [PATCH 5/6] Run workflows on PRs as well (#649) --- .github/workflows/ci.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2e931917..31283ab1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,7 +1,6 @@ name: CI -on: - push: +on: [push, pull_request] jobs: CI: From 34185ee39982a3430bf4883b240a705a6d77a23f Mon Sep 17 00:00:00 2001 From: "Chanyub.Park" Date: Tue, 3 Sep 2024 19:02:58 +0900 Subject: [PATCH 6/6] fix lint --- object_store_factory/src/local.rs | 34 ++++++++++++++++++------------- src/main.rs | 2 +- src/object_store/wrapped.rs | 31 +++++++++++++++++++++++----- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/object_store_factory/src/local.rs b/object_store_factory/src/local.rs index 9d107a42..3d70109f 100644 --- a/object_store_factory/src/local.rs +++ b/object_store_factory/src/local.rs @@ -19,20 +19,26 @@ impl LocalConfig { map: &HashMap, ) -> Result { Ok(Self { - data_dir: map.get("data_dir") - .ok_or_else(|| object_store::Error::Generic { - store: "local", - source: "Missing data_dir".into(), - })? - .clone(), - disable_hardlinks: map.get("disable_hardlinks").map(|s| s == "true").unwrap_or(false), + data_dir: map + .get("data_dir") + .ok_or_else(|| object_store::Error::Generic { + store: "local", + source: "Missing data_dir".into(), + })? + .clone(), + disable_hardlinks: map + .get("disable_hardlinks") + .map(|s| s == "true") + .unwrap_or(false), }) } pub fn to_hashmap(&self) -> HashMap { let mut map = HashMap::new(); map.insert("data_dir".to_string(), self.data_dir.clone()); - map.insert("disable_hardlinks".to_string(), self.disable_hardlinks.to_string()); + map.insert( + "disable_hardlinks".to_string(), + self.disable_hardlinks.to_string()); map } @@ -57,7 +63,7 @@ mod tests { let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.disable_hardlinks, false); // Default value + assert!(!config.disable_hardlinks); // Default value } #[test] @@ -69,7 +75,7 @@ mod tests { let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.disable_hardlinks, true); + assert!(config.disable_hardlinks); } #[test] @@ -81,7 +87,7 @@ mod tests { let config = LocalConfig::from_hashmap(&map) .expect("Failed to create config from hashmap"); assert_eq!(config.data_dir, "/tmp/data".to_string()); - assert_eq!(config.disable_hardlinks, false); + assert!(!config.disable_hardlinks); } #[test] @@ -133,7 +139,7 @@ mod tests { #[test] fn test_default_false() { - assert_eq!(default_false(), false); + assert!(!default_false()); } #[test] @@ -146,7 +152,7 @@ mod tests { let config: LocalConfig = serde_json::from_str(json).unwrap(); assert_eq!(config.data_dir, "/tmp/data"); - assert_eq!(config.disable_hardlinks, false); + assert!(!config.disable_hardlinks); } #[test] @@ -160,6 +166,6 @@ mod tests { let config: LocalConfig = serde_json::from_str(json).unwrap(); assert_eq!(config.data_dir, "/tmp/data"); - assert_eq!(config.disable_hardlinks, true); + assert!(config.disable_hardlinks); } } \ No newline at end of file diff --git a/src/main.rs b/src/main.rs index 46eab5f4..1caf26e7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,7 +29,7 @@ use seafowl::{ use tokio::time::{interval, Duration}; use tracing::level_filters::LevelFilter; -use tracing::{error, info, debug, subscriber, warn}; +use tracing::{debug, error, info, subscriber, warn}; use tracing_log::LogTracer; use tracing_subscriber::filter::EnvFilter; diff --git a/src/object_store/wrapped.rs b/src/object_store/wrapped.rs index ad4446db..66dd06a8 100644 --- a/src/object_store/wrapped.rs +++ b/src/object_store/wrapped.rs @@ -16,8 +16,8 @@ use url::Url; use object_store_factory::aws::S3Config; use object_store_factory::google::GCSConfig; -use object_store_factory::ObjectStoreConfig; use object_store_factory::local::LocalConfig; +use object_store_factory::ObjectStoreConfig; // Wrapper around the object_store crate that holds on to the original config // in order to provide a more efficient "upload" for the local object store @@ -152,8 +152,21 @@ impl ObjectStore for InternalObjectStore { payload: PutPayload, opts: PutOptions, ) -> Result { - if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { - return self.inner.put_opts(location, payload, PutOptions{mode: object_store::PutMode::Overwrite, ..opts}).await + if let ObjectStoreConfig::Local(LocalConfig { + disable_hardlinks: true, + .. + }) = self.config + { + return self + .inner + .put_opts( + location, + payload, + PutOptions{ + mode: object_store::PutMode::Overwrite, + ..opts + }, + ).await; }; self.inner.put_opts(location, payload, opts).await } @@ -243,7 +256,11 @@ impl ObjectStore for InternalObjectStore { /// /// Will return an error if the destination already has an object. async fn copy_if_not_exists(&self, from: &Path, to: &Path) -> Result<()> { - if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { + if let ObjectStoreConfig::Local(LocalConfig { + disable_hardlinks: true, + .. + }) = self.config + { return self.inner.copy(from, to).await; } self.inner.copy_if_not_exists(from, to).await @@ -261,7 +278,11 @@ impl ObjectStore for InternalObjectStore { // this with a lock too, so look into using that down the line instead. return self.inner.rename(from, to).await; } - if let ObjectStoreConfig::Local(LocalConfig { disable_hardlinks: true, .. }) = self.config { + if let ObjectStoreConfig::Local(LocalConfig { + disable_hardlinks: true, + .. + }) = self.config + { return self.inner.rename(from, to).await; } self.inner.rename_if_not_exists(from, to).await