From 27561e2a21354b25218030fff97dc703e3b453f7 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 26 Feb 2021 14:36:35 -0500 Subject: [PATCH 1/3] refactor: Move logging out of object store configuration validation --- src/influxdb_ioxd.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/influxdb_ioxd.rs b/src/influxdb_ioxd.rs index 32a7ce1900..1329099155 100644 --- a/src/influxdb_ioxd.rs +++ b/src/influxdb_ioxd.rs @@ -98,13 +98,21 @@ pub async fn main(logging_level: LoggingLevel, config: Option) -> Result let f = SendPanicsToTracing::new(); std::mem::forget(f); + match config.object_store { + Some(ObjStoreOpt::Memory) | None => { + warn!("NO PERSISTENCE: using Memory for object storage"); + } + Some(store) => { + info!("Using {} for object storage", store); + } + } + let object_store = match ( config.object_store, config.bucket, config.database_directory, ) { (Some(ObjStoreOpt::Google), Some(bucket), _) => { - info!("Using GCP bucket {} for storage", bucket); ObjectStore::new_google_cloud_storage(GoogleCloudStorage::new(bucket)) } (Some(ObjStoreOpt::Google), None, _) => { @@ -114,7 +122,6 @@ pub async fn main(logging_level: LoggingLevel, config: Option) -> Result .fail(); } (Some(ObjStoreOpt::S3), Some(bucket), _) => { - info!("Using S3 bucket {} for storage", bucket); // rusoto::Region's default takes the value from the AWS_DEFAULT_REGION env var. ObjectStore::new_amazon_s3(AmazonS3::new(Default::default(), bucket)) } @@ -125,7 +132,6 @@ pub async fn main(logging_level: LoggingLevel, config: Option) -> Result .fail(); } (Some(ObjStoreOpt::File), _, Some(ref db_dir)) => { - info!("Using local dir {:?} for storage", db_dir); fs::create_dir_all(db_dir).context(CreatingDatabaseDirectory { path: db_dir })?; ObjectStore::new_file(object_store::disk::File::new(&db_dir)) } @@ -142,7 +148,6 @@ pub async fn main(logging_level: LoggingLevel, config: Option) -> Result .fail(); } (Some(ObjStoreOpt::Memory), _, _) | (None, _, _) => { - warn!("NO PERSISTENCE: using memory for object storage"); ObjectStore::new_in_memory(object_store::memory::InMemory::new()) } }; From daacae5edccfa4cb87e5e316bed75c0d610e0b80 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 26 Feb 2021 16:41:14 -0500 Subject: [PATCH 2/3] refactor: Extract a function for validating object store configs Bonus: easier to write some tests! Only writing some for default/memory in this commit, the other object stores are about to change. --- src/influxdb_ioxd.rs | 149 ++++++++++++++++++++++++++----------------- 1 file changed, 92 insertions(+), 57 deletions(-) diff --git a/src/influxdb_ioxd.rs b/src/influxdb_ioxd.rs index 1329099155..29d099cf6c 100644 --- a/src/influxdb_ioxd.rs +++ b/src/influxdb_ioxd.rs @@ -1,20 +1,14 @@ -use std::fs; -use std::net::SocketAddr; -use std::path::PathBuf; -use std::sync::Arc; - -use hyper::Server; -use snafu::{ResultExt, Snafu}; -use tracing::{error, info, warn}; - -use object_store::{self, aws::AmazonS3, gcp::GoogleCloudStorage, ObjectStore}; -use panic_logging::SendPanicsToTracing; -use server::{ConnectionManagerImpl as ConnectionManager, Server as AppServer}; - use crate::commands::{ logging::LoggingLevel, server::{load_config, Config, ObjectStore as ObjStoreOpt}, }; +use hyper::Server; +use object_store::{self, aws::AmazonS3, gcp::GoogleCloudStorage, ObjectStore}; +use panic_logging::SendPanicsToTracing; +use server::{ConnectionManagerImpl as ConnectionManager, Server as AppServer}; +use snafu::{ResultExt, Snafu}; +use std::{convert::TryFrom, fs, net::SocketAddr, path::PathBuf, sync::Arc}; +use tracing::{error, info, warn}; mod http; mod rpc; @@ -107,50 +101,7 @@ pub async fn main(logging_level: LoggingLevel, config: Option) -> Result } } - let object_store = match ( - config.object_store, - config.bucket, - config.database_directory, - ) { - (Some(ObjStoreOpt::Google), Some(bucket), _) => { - ObjectStore::new_google_cloud_storage(GoogleCloudStorage::new(bucket)) - } - (Some(ObjStoreOpt::Google), None, _) => { - return InvalidCloudObjectStoreConfiguration { - object_store: ObjStoreOpt::Google, - } - .fail(); - } - (Some(ObjStoreOpt::S3), Some(bucket), _) => { - // rusoto::Region's default takes the value from the AWS_DEFAULT_REGION env var. - ObjectStore::new_amazon_s3(AmazonS3::new(Default::default(), bucket)) - } - (Some(ObjStoreOpt::S3), None, _) => { - return InvalidCloudObjectStoreConfiguration { - object_store: ObjStoreOpt::S3, - } - .fail(); - } - (Some(ObjStoreOpt::File), _, Some(ref db_dir)) => { - fs::create_dir_all(db_dir).context(CreatingDatabaseDirectory { path: db_dir })?; - ObjectStore::new_file(object_store::disk::File::new(&db_dir)) - } - (Some(ObjStoreOpt::File), _, None) => { - return InvalidFileObjectStoreConfiguration.fail(); - } - (Some(ObjStoreOpt::Azure), Some(_bucket), _) => { - unimplemented!(); - } - (Some(ObjStoreOpt::Azure), None, _) => { - return InvalidCloudObjectStoreConfiguration { - object_store: ObjStoreOpt::Azure, - } - .fail(); - } - (Some(ObjStoreOpt::Memory), _, _) | (None, _, _) => { - ObjectStore::new_in_memory(object_store::memory::InMemory::new()) - } - }; + let object_store = ObjectStore::try_from(&config)?; let object_storage = Arc::new(object_store); let connection_manager = ConnectionManager {}; @@ -202,3 +153,87 @@ pub async fn main(logging_level: LoggingLevel, config: Option) -> Result Ok(()) } + +impl TryFrom<&Config> for ObjectStore { + type Error = Error; + + fn try_from(config: &Config) -> Result { + let os = match ( + config.object_store, + config.bucket.as_ref(), + config.database_directory.as_ref(), + ) { + (Some(ObjStoreOpt::Google), Some(bucket), _) => { + Self::new_google_cloud_storage(GoogleCloudStorage::new(bucket)) + } + (Some(ObjStoreOpt::Google), None, _) => { + return InvalidCloudObjectStoreConfiguration { + object_store: ObjStoreOpt::Google, + } + .fail(); + } + (Some(ObjStoreOpt::S3), Some(bucket), _) => { + // rusoto::Region's default takes the value from the AWS_DEFAULT_REGION env var. + Self::new_amazon_s3(AmazonS3::new(Default::default(), bucket)) + } + (Some(ObjStoreOpt::S3), None, _) => { + return InvalidCloudObjectStoreConfiguration { + object_store: ObjStoreOpt::S3, + } + .fail(); + } + (Some(ObjStoreOpt::File), _, Some(ref db_dir)) => { + fs::create_dir_all(db_dir).context(CreatingDatabaseDirectory { path: db_dir })?; + Self::new_file(object_store::disk::File::new(&db_dir)) + } + (Some(ObjStoreOpt::File), _, None) => { + return InvalidFileObjectStoreConfiguration.fail(); + } + (Some(ObjStoreOpt::Azure), Some(_bucket), _) => { + unimplemented!(); + } + (Some(ObjStoreOpt::Azure), None, _) => { + return InvalidCloudObjectStoreConfiguration { + object_store: ObjStoreOpt::Azure, + } + .fail(); + } + (Some(ObjStoreOpt::Memory), _, _) | (None, _, _) => { + Self::new_in_memory(object_store::memory::InMemory::new()) + } + }; + + Ok(os) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use object_store::ObjectStoreIntegration; + use structopt::StructOpt; + + #[test] + fn default_object_store_is_memory() { + let config = Config::from_iter_safe(&["server"]).unwrap(); + + let object_store = ObjectStore::try_from(&config).unwrap(); + + assert!(matches!( + object_store, + ObjectStore(ObjectStoreIntegration::InMemory(_)) + )); + } + + #[test] + fn explicitly_set_object_store_to_memory() { + let config = Config::from_iter_safe(&["server", "--object-store", "memory"]).unwrap(); + + let object_store = ObjectStore::try_from(&config).unwrap(); + + assert!(matches!( + object_store, + ObjectStore(ObjectStoreIntegration::InMemory(_)) + )); + } +} From 457747c6c43598fee89d46f6ab323282e28e805d Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 26 Feb 2021 16:48:01 -0500 Subject: [PATCH 3/3] refactor: Rearrange object store config validation to be differently scoped There were already too many items in the top-level match that were being ignored in some match arms, and I'm about to add more, so switch to an MLM (Multi-Level Match, not Multi-Level Marketing ;)) --- src/influxdb_ioxd.rs | 79 +++++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/src/influxdb_ioxd.rs b/src/influxdb_ioxd.rs index 29d099cf6c..0e12c69ee4 100644 --- a/src/influxdb_ioxd.rs +++ b/src/influxdb_ioxd.rs @@ -158,52 +158,55 @@ impl TryFrom<&Config> for ObjectStore { type Error = Error; fn try_from(config: &Config) -> Result { - let os = match ( - config.object_store, - config.bucket.as_ref(), - config.database_directory.as_ref(), - ) { - (Some(ObjStoreOpt::Google), Some(bucket), _) => { - Self::new_google_cloud_storage(GoogleCloudStorage::new(bucket)) + match config.object_store { + Some(ObjStoreOpt::Memory) | None => { + Ok(Self::new_in_memory(object_store::memory::InMemory::new())) } - (Some(ObjStoreOpt::Google), None, _) => { - return InvalidCloudObjectStoreConfiguration { + + Some(ObjStoreOpt::Google) => match config.bucket.as_ref() { + Some(bucket) => Ok(Self::new_google_cloud_storage(GoogleCloudStorage::new( + bucket, + ))), + None => InvalidCloudObjectStoreConfiguration { object_store: ObjStoreOpt::Google, } - .fail(); - } - (Some(ObjStoreOpt::S3), Some(bucket), _) => { - // rusoto::Region's default takes the value from the AWS_DEFAULT_REGION env var. - Self::new_amazon_s3(AmazonS3::new(Default::default(), bucket)) - } - (Some(ObjStoreOpt::S3), None, _) => { - return InvalidCloudObjectStoreConfiguration { - object_store: ObjStoreOpt::S3, + .fail(), + }, + + Some(ObjStoreOpt::S3) => { + match config.bucket.as_ref() { + Some(bucket) => { + // rusoto::Region's default takes the value from the AWS_DEFAULT_REGION env + // var. + Ok(Self::new_amazon_s3(AmazonS3::new( + Default::default(), + bucket, + ))) + } + None => InvalidCloudObjectStoreConfiguration { + object_store: ObjStoreOpt::S3, + } + .fail(), } - .fail(); } - (Some(ObjStoreOpt::File), _, Some(ref db_dir)) => { - fs::create_dir_all(db_dir).context(CreatingDatabaseDirectory { path: db_dir })?; - Self::new_file(object_store::disk::File::new(&db_dir)) - } - (Some(ObjStoreOpt::File), _, None) => { - return InvalidFileObjectStoreConfiguration.fail(); - } - (Some(ObjStoreOpt::Azure), Some(_bucket), _) => { - unimplemented!(); - } - (Some(ObjStoreOpt::Azure), None, _) => { - return InvalidCloudObjectStoreConfiguration { + + Some(ObjStoreOpt::Azure) => match config.bucket.as_ref() { + Some(_bucket) => unimplemented!(), + None => InvalidCloudObjectStoreConfiguration { object_store: ObjStoreOpt::Azure, } - .fail(); - } - (Some(ObjStoreOpt::Memory), _, _) | (None, _, _) => { - Self::new_in_memory(object_store::memory::InMemory::new()) - } - }; + .fail(), + }, - Ok(os) + Some(ObjStoreOpt::File) => match config.database_directory.as_ref() { + Some(db_dir) => { + fs::create_dir_all(db_dir) + .context(CreatingDatabaseDirectory { path: db_dir })?; + Ok(Self::new_file(object_store::disk::File::new(&db_dir))) + } + None => InvalidFileObjectStoreConfiguration.fail(), + }, + } } }