From b0cb300af8ac411914a1dfd75655361c5196814d Mon Sep 17 00:00:00 2001 From: Stanislau Hlebik Date: Mon, 9 Mar 2020 12:03:07 -0700 Subject: [PATCH] mononoke: introduce BlobrepoBuilder Summary: The goal of the whole stack is quite simple (add reponame field to BlobRepo), but this stack also tries to make it easier to initialize BlobRepo. To do that BlobrepoBuilder was added. It now accepts RepoConfig instead of 6 different fields from RepoConfig - that makes it easier to pass a field from config into BlobRepo. It also allows to customize BlobRepo. Currently it's used just to add redaction override, but later we can extend it for other use cases as well, with the hope that we'll be able to remove a bunch of repo-creation functions from cmdlib. Because of BlobrepoBuilder we no longer need open_blobrepo function. Later we might consider removing open_blobrepo_given_datasources as well. Note that this diff *adds* a few new clones. I don't consider it being a big problem, though I'm curious to hear your thoughts folks. Note that another option for the implementation would be to take a reference to objects instead of taking them by value. I briefly looked into how they used, and lot of them are passed to the objects that actually take ownership of what's inside these config fields. I.e. Blobstore essentially takes ownership of BlobstoreOptions, because it needs to store manifold bucket name. Same for scuba_censored_table, filestore_params, bookmarks_cache_ttl etc. So unless I'm missing anything, we can either pass them as reference and then we'll have to copy them, or we can just pass a value from BlobrepoBuilder directly. Reviewed By: krallin Differential Revision: D20312567 fbshipit-source-id: 14634f5e14f103b110482557254f084da1c725e1 --- eden/mononoke/apiserver/src/actor/repo.rs | 16 +- eden/mononoke/blobrepo/factory/src/lib.rs | 140 ++++++++++++------ eden/mononoke/cmdlib/src/args.rs | 18 +-- eden/mononoke/hook_tailer/main.rs | 17 +-- eden/mononoke/lfs_server/src/main.rs | 31 ++-- eden/mononoke/microwave/builder/main.rs | 30 +--- eden/mononoke/mononoke_api/src/repo.rs | 17 +-- .../repo_client/mononoke_repo/src/builder.rs | 15 +- 8 files changed, 144 insertions(+), 140 deletions(-) diff --git a/eden/mononoke/apiserver/src/actor/repo.rs b/eden/mononoke/apiserver/src/actor/repo.rs index 92ef480681..a3710e7720 100644 --- a/eden/mononoke/apiserver/src/actor/repo.rs +++ b/eden/mononoke/apiserver/src/actor/repo.rs @@ -14,7 +14,7 @@ use std::{ use anyhow::{format_err, Error}; use blobrepo::{file_history::get_file_history, BlobRepo}; -use blobrepo_factory::{open_blobrepo, BlobstoreOptions, Caching, ReadOnlyStorage}; +use blobrepo_factory::{BlobrepoBuilder, BlobstoreOptions, Caching, ReadOnlyStorage}; use blobstore::Loadable; use bookmarks::BookmarkName; use cloned::cloned; @@ -130,7 +130,6 @@ impl MononokeRepo { let skiplist_index_blobstore_key = config.skiplist_index_blobstore_key.clone(); - let repoid = config.repoid; let monitoring_config = config.source_control_service_monitoring.clone(); let commit_sync_config = config.commit_sync_config.clone(); @@ -148,22 +147,17 @@ impl MononokeRepo { { cloned!(logger); async move { - open_blobrepo( + let builder = BlobrepoBuilder::new( fb, - config.storage_config, - repoid, + &config, mysql_options, with_cachelib, - config.bookmarks_cache_ttl, - config.redaction, common_config.scuba_censored_table, - config.filestore, readonly_storage, blobstore_options, &logger, - config.derived_data_config, - ) - .await + ); + builder.build().await } } .boxed() diff --git a/eden/mononoke/blobrepo/factory/src/lib.rs b/eden/mononoke/blobrepo/factory/src/lib.rs index fd78da5da2..b072dd74b8 100644 --- a/eden/mononoke/blobrepo/factory/src/lib.rs +++ b/eden/mononoke/blobrepo/factory/src/lib.rs @@ -38,7 +38,7 @@ use git_types::TreeHandle; use maplit::btreeset; use memblob::EagerMemblob; use metaconfig_types::{ - self, DerivedDataConfig, FilestoreParams, Redaction, StorageConfig, UnodeVersion, + self, DerivedDataConfig, FilestoreParams, Redaction, RepoConfig, StorageConfig, UnodeVersion, }; use mononoke_types::RepositoryId; use newfilenodes::NewFilenodesBuilder; @@ -65,14 +65,7 @@ pub enum Caching { const BLOBSTORE_BLOBS_CACHE_POOL: &'static str = "blobstore-blobs"; const BLOBSTORE_PRESENCE_CACHE_POOL: &'static str = "blobstore-presence"; -/// Construct a new BlobRepo with the given storage configuration. If the metadata DB is -/// remote (ie, MySQL), then it configures a full set of caches. Otherwise with local storage -/// it's assumed to be a test configuration. -/// -/// The blobstore config is actually orthogonal to this, but it wouldn't make much sense to -/// configure a local blobstore with a remote db, or vice versa. There's no error checking -/// at this level (aside from disallowing a multiplexed blobstore with a local db). -pub async fn open_blobrepo( +pub struct BlobrepoBuilder<'a> { fb: FacebookInit, storage_config: StorageConfig, repoid: RepositoryId, @@ -84,45 +77,102 @@ pub async fn open_blobrepo( filestore_params: Option, readonly_storage: ReadOnlyStorage, blobstore_options: BlobstoreOptions, - logger: &Logger, + logger: &'a Logger, derived_data_config: DerivedDataConfig, -) -> Result { - let sql_factory = make_sql_factory( - fb, - storage_config.dbconfig, - mysql_options, - readonly_storage, - // FIXME: remove clone when mysql_sql_factory is async-await - logger.clone(), - ) - .boxify(); +} - let blobstore = make_blobstore( - fb, - storage_config.blobstore, - mysql_options, - readonly_storage, - blobstore_options, - // FIXME: remove clone when make_blobstore is async-await - logger.clone(), - ) - .boxify(); +impl<'a> BlobrepoBuilder<'a> { + pub fn new( + fb: FacebookInit, + config: &RepoConfig, + mysql_options: MysqlOptions, + caching: Caching, + scuba_censored_table: Option, + readonly_storage: ReadOnlyStorage, + blobstore_options: BlobstoreOptions, + logger: &'a Logger, + ) -> Self { + Self { + fb, + storage_config: config.storage_config.clone(), + repoid: config.repoid, + mysql_options, + caching, + bookmarks_cache_ttl: config.bookmarks_cache_ttl.clone(), + redaction: config.redaction.clone(), + scuba_censored_table, + filestore_params: config.filestore.clone(), + readonly_storage, + blobstore_options, + logger, + derived_data_config: config.derived_data_config.clone(), + } + } - open_blobrepo_given_datasources( - fb, - blobstore, - sql_factory, - repoid, - caching, - bookmarks_cache_ttl, - redaction, - scuba_censored_table, - filestore_params, - readonly_storage, - derived_data_config, - ) - .compat() - .await + pub fn set_redaction(&mut self, redaction: Redaction) { + self.redaction = redaction; + } + + /// remote (ie, MySQL), then it configures a full set of caches. Otherwise with local storage + /// it's assumed to be a test configuration. + /// + /// The blobstore config is actually orthogonal to this, but it wouldn't make much sense to + /// configure a local blobstore with a remote db, or vice versa. There's no error checking + /// at this level (aside from disallowing a multiplexed blobstore with a local db). + pub async fn build(self) -> Result { + let BlobrepoBuilder { + fb, + storage_config, + repoid, + mysql_options, + caching, + bookmarks_cache_ttl, + redaction, + scuba_censored_table, + filestore_params, + readonly_storage, + blobstore_options, + logger, + derived_data_config, + } = self; + + let sql_factory = make_sql_factory( + fb, + storage_config.dbconfig, + mysql_options, + readonly_storage, + // FIXME: remove clone when mysql_sql_factory is async-await + logger.clone(), + ) + .boxify(); + + let blobstore = make_blobstore( + fb, + storage_config.blobstore, + mysql_options, + readonly_storage, + blobstore_options, + // FIXME: remove clone when make_blobstore is async-await + logger.clone(), + ) + .boxify(); + + open_blobrepo_given_datasources( + fb, + blobstore, + sql_factory, + repoid, + caching, + bookmarks_cache_ttl, + redaction, + scuba_censored_table, + filestore_params, + readonly_storage, + derived_data_config, + ) + .compat() + .await + } } /// Expose for graph walker that has storage open already diff --git a/eden/mononoke/cmdlib/src/args.rs b/eden/mononoke/cmdlib/src/args.rs index a2881f32d2..3d8c67a313 100644 --- a/eden/mononoke/cmdlib/src/args.rs +++ b/eden/mononoke/cmdlib/src/args.rs @@ -31,7 +31,7 @@ use slog_term::TermDecorator; use slog_glog_fmt::{kv_categorizer::FacebookCategorizer, kv_defaults::FacebookKV, GlogFormat}; use blobrepo::BlobRepo; -use blobrepo_factory::{open_blobrepo, Caching, ReadOnlyStorage}; +use blobrepo_factory::{BlobrepoBuilder, Caching, ReadOnlyStorage}; use blobstore_factory::{BlobstoreOptions, ChaosOptions, Scrubbing, ThrottleOptions}; use changesets::SqlConstructors; use metaconfig_parser::RepoConfigs; @@ -1039,22 +1039,20 @@ fn open_repo_internal_with_repo_id<'a>( cloned!(logger); async move { - open_blobrepo( + let mut builder = BlobrepoBuilder::new( fb, - config.storage_config, - repo_id, + &config, mysql_options, caching, - config.bookmarks_cache_ttl, - redaction_override.unwrap_or(config.redaction), common_config.scuba_censored_table, - config.filestore, readonly_storage, blobstore_options, &logger, - config.derived_data_config, - ) - .await + ); + if let Some(redaction_override) = redaction_override { + builder.set_redaction(redaction_override); + } + builder.build().await } .boxed() .compat() diff --git a/eden/mononoke/hook_tailer/main.rs b/eden/mononoke/hook_tailer/main.rs index e248841593..e1e1aea8f2 100644 --- a/eden/mononoke/hook_tailer/main.rs +++ b/eden/mononoke/hook_tailer/main.rs @@ -11,7 +11,7 @@ pub mod tailer; use anyhow::{format_err, Error, Result}; -use blobrepo_factory::open_blobrepo; +use blobrepo_factory::BlobrepoBuilder; use bookmarks::BookmarkName; use clap::{App, Arg, ArgMatches}; use cloned::cloned; @@ -87,23 +87,18 @@ fn main(fb: FacebookInit) -> Result<()> { let caching = cmdlib::args::init_cachelib(fb, &matches, None); let readonly_storage = cmdlib::args::parse_readonly_storage(&matches); - let blobrepo = open_blobrepo( + let builder = BlobrepoBuilder::new( fb, - config.storage_config.clone(), - config.repoid, + &config, cmdlib::args::parse_mysql_options(&matches), caching, - config.bookmarks_cache_ttl, - config.redaction, common_config.scuba_censored_table, - config.filestore.clone(), readonly_storage, cmdlib::args::parse_blobstore_options(&matches), &logger, - config.derived_data_config.clone(), - ) - .boxed() - .compat(); + ); + + let blobrepo = builder.build().boxed().compat(); let rc = RequestContext { bucket_name: "mononoke_prod".into(), diff --git a/eden/mononoke/lfs_server/src/main.rs b/eden/mononoke/lfs_server/src/main.rs index 634e74eaf5..a8c50396f7 100644 --- a/eden/mononoke/lfs_server/src/main.rs +++ b/eden/mononoke/lfs_server/src/main.rs @@ -35,7 +35,7 @@ use tokio_old::net::TcpListener; use tokio_openssl::SslAcceptorExt; use blobrepo::BlobRepo; -use blobrepo_factory::open_blobrepo; +use blobrepo_factory::BlobrepoBuilder; use cmdlib::{ args::{self, get_config_handle}, helpers::serve_forever, @@ -256,6 +256,17 @@ fn main(fb: FacebookInit) -> Result<(), Error> { let scuba_censored_table = common.scuba_censored_table.clone(); cloned!(blobstore_options, test_acl_checker, logger); async move { + let builder = BlobrepoBuilder::new( + fb, + &config, + mysql_options, + caching, + scuba_censored_table, + readonly_storage, + blobstore_options, + &logger, + ); + let hipster_acl = config.hipster_acl; let aclchecker = async { if let Some(test_checker) = test_acl_checker { @@ -265,23 +276,7 @@ fn main(fb: FacebookInit) -> Result<(), Error> { } }; - let blobrepo = open_blobrepo( - fb, - config.storage_config, - config.repoid, - mysql_options, - caching, - config.bookmarks_cache_ttl, - config.redaction, - scuba_censored_table, - config.filestore, - readonly_storage, - blobstore_options, - &logger, - config.derived_data_config, - ); - - let (repo, aclchecker) = try_join!(blobrepo, aclchecker)?; + let (repo, aclchecker) = try_join!(builder.build(), aclchecker)?; Result::<(String, (BlobRepo, LfsAclChecker)), Error>::Ok((name, (repo, aclchecker))) } diff --git a/eden/mononoke/microwave/builder/main.rs b/eden/mononoke/microwave/builder/main.rs index 4bdc1ce128..f2ba2c6fa8 100644 --- a/eden/mononoke/microwave/builder/main.rs +++ b/eden/mononoke/microwave/builder/main.rs @@ -12,7 +12,7 @@ use ::changesets::Changesets; use ::filenodes::Filenodes; use anyhow::{format_err, Error}; use blobrepo::DangerousOverride; -use blobrepo_factory::open_blobrepo; +use blobrepo_factory::BlobrepoBuilder; use clap::{Arg, ArgMatches, SubCommand}; use cloned::cloned; use cmdlib::{args, monitoring::AliveService}; @@ -20,7 +20,6 @@ use context::SessionContainer; use fbinit::FacebookInit; use futures::{channel::mpsc, compat::Future01CompatExt, future}; use metaconfig_parser::RepoConfigs; -use metaconfig_types::RepoConfig; use microwave::{Snapshot, SnapshotLocation}; use slog::{info, o, Logger}; use std::path::Path; @@ -78,35 +77,20 @@ async fn do_main<'a>( let (changesets_sender, changesets_receiver) = mpsc::channel(1000); let warmup_ctx = ctx.clone(); - let RepoConfig { - storage_config, - repoid, - bookmarks_cache_ttl, - redaction, - filestore, - derived_data_config, - cache_warmup, - .. - } = config; - let warmup = async move { - let repo = open_blobrepo( + let builder = BlobrepoBuilder::new( fb, - storage_config, - repoid, + &config, mysql_options, caching, - bookmarks_cache_ttl, - redaction, scuba_censored_table, - filestore, readonly_storage, blobstore_options, &logger, - derived_data_config, - ) - .await?; + ); + let repo = builder.build().await?; + let repoid = config.repoid; let warmup_repo = repo .dangerous_override(|inner| -> Arc { Arc::new(MicrowaveFilenodes::new(repoid, filenodes_sender, inner)) @@ -115,7 +99,7 @@ async fn do_main<'a>( Arc::new(MicrowaveChangesets::new(repoid, changesets_sender, inner)) }); - cache_warmup::cache_warmup(warmup_ctx, warmup_repo, cache_warmup) + cache_warmup::cache_warmup(warmup_ctx, warmup_repo, config.cache_warmup) .compat() .await?; diff --git a/eden/mononoke/mononoke_api/src/repo.rs b/eden/mononoke/mononoke_api/src/repo.rs index c6c50c50ad..37c2b0c66f 100644 --- a/eden/mononoke/mononoke_api/src/repo.rs +++ b/eden/mononoke/mononoke_api/src/repo.rs @@ -14,7 +14,7 @@ use std::{ use aclchecker::AclChecker; use anyhow::{bail, format_err, Error}; use blobrepo::BlobRepo; -use blobrepo_factory::{open_blobrepo, BlobstoreOptions, Caching, ReadOnlyStorage}; +use blobrepo_factory::{BlobrepoBuilder, BlobstoreOptions, Caching, ReadOnlyStorage}; use blobstore::Loadable; use blobstore_factory::make_sql_factory; use bookmarks::{BookmarkName, BookmarkPrefix}; @@ -126,8 +126,6 @@ impl Repo { ) -> Result { let skiplist_index_blobstore_key = config.skiplist_index_blobstore_key.clone(); - let repoid = config.repoid; - let synced_commit_mapping = open_synced_commit_mapping( fb, config.clone(), @@ -139,22 +137,17 @@ impl Repo { let service_config = config.source_control_service.clone(); let monitoring_config = config.source_control_service_monitoring.clone(); - let blob_repo = open_blobrepo( + let builder = BlobrepoBuilder::new( fb, - config.storage_config.clone(), - repoid, + &config, mysql_options, with_cachelib, - config.bookmarks_cache_ttl, - config.redaction, common_config.scuba_censored_table, - config.filestore, readonly_storage, blobstore_options, &logger, - config.derived_data_config, - ) - .await?; + ); + let blob_repo = builder.build().await?; let ctx = CoreContext::new_with_logger(fb, logger.clone()); diff --git a/eden/mononoke/repo_client/mononoke_repo/src/builder.rs b/eden/mononoke/repo_client/mononoke_repo/src/builder.rs index 174523b74c..c259a16cf5 100644 --- a/eden/mononoke/repo_client/mononoke_repo/src/builder.rs +++ b/eden/mononoke/repo_client/mononoke_repo/src/builder.rs @@ -7,7 +7,7 @@ use anyhow::Error; use blobrepo::BlobRepo; -use blobrepo_factory::{open_blobrepo, BlobstoreOptions, Caching, ReadOnlyStorage}; +use blobrepo_factory::{BlobrepoBuilder, BlobstoreOptions, Caching, ReadOnlyStorage}; use context::CoreContext; use futures::{compat::Future01CompatExt, future}; use hooks::HookManager; @@ -41,22 +41,17 @@ impl MononokeRepoBuilder { readonly_storage: ReadOnlyStorage, blobstore_options: BlobstoreOptions, ) -> Result { - let repo = open_blobrepo( + let builder = BlobrepoBuilder::new( ctx.fb, - config.storage_config.clone(), - config.repoid, + &config, mysql_options, caching, - config.bookmarks_cache_ttl, - config.redaction, scuba_censored_table.clone(), - config.filestore.clone(), readonly_storage, blobstore_options.clone(), ctx.logger(), - config.derived_data_config.clone(), - ) - .await?; + ); + let repo = builder.build().await?; Ok(Self { ctx,