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
This commit is contained in:
Stanislau Hlebik 2020-03-09 12:03:07 -07:00 committed by Facebook Github Bot
parent 44effbe62e
commit b0cb300af8
8 changed files with 144 additions and 140 deletions

View File

@ -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()

View File

@ -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<FilestoreParams>,
readonly_storage: ReadOnlyStorage,
blobstore_options: BlobstoreOptions,
logger: &Logger,
logger: &'a Logger,
derived_data_config: DerivedDataConfig,
) -> Result<BlobRepo, Error> {
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<String>,
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<BlobRepo, Error> {
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

View File

@ -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()

View File

@ -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(),

View File

@ -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)))
}

View File

@ -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<dyn Filenodes> {
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?;

View File

@ -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<Self, Error> {
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());

View File

@ -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<MononokeRepoBuilder, Error> {
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,