Allow passing a custom database when creating the Service #3957
Conversation
| @@ -157,15 +157,6 @@ where TGen: RuntimeGenesis, TCSExt: Extension { | |||
| >, Error> { | |||
| let keystore = Keystore::open(config.keystore_path.clone(), config.keystore_password.clone())?; | |||
|
|
|||
| let db_settings = client_db::DatabaseSettings { | |||
| cache_size: None, | |||
There was a problem hiding this comment.
Note that this was surprisingly None rather than config.cache_size.
Apparently this was not intended, so I fixed it.
cc @svyatonik
| @@ -32,6 +32,7 @@ header-metadata = { package = "substrate-header-metadata", path = "header-metada | |||
| [dev-dependencies] | |||
| env_logger = "0.7.0" | |||
| tempfile = "3.1.0" | |||
| client-db = { package = "substrate-client-db", path = "./db", features = ["kvdb-rocksdb"] } | |||
There was a problem hiding this comment.
Is this really needed? test-client already pulls in client_db
There was a problem hiding this comment.
I added this in order to force the kvdb-rocksdb feature, otherwise the test fails.
Alternatively, I could maybe change the test to use an in-memory database.
core/cli/src/lib.rs
Outdated
| let db_path = match config.database { | ||
| ConfigurationDb::Path { path, .. } => path, | ||
| _ => { | ||
| println!("Cannot purge custom database implementation"); |
There was a problem hiding this comment.
I went for println! because this is a CLI function, and the rest of the function also uses println! several times.
There was a problem hiding this comment.
Normally errors should be printed to stderr.
core/service/src/config.rs
Outdated
| @@ -96,6 +94,23 @@ pub struct Configuration<C, G, E = NoExtension> { | |||
| pub dev_key_seed: Option<String>, | |||
| } | |||
|
|
|||
| /// Configuration of the database of the client. | |||
| #[derive(Clone)] | |||
| pub enum ConfigurationDb { | |||
There was a problem hiding this comment.
DbConfiguration or even DatabaseConfig would match our coding guide better
|
LG in general, minor grumbles |
core/client/db/src/lib.rs
Outdated
| cache_size: Option<usize>, | ||
| }, | ||
|
|
||
| /// Use a custom already-open database. Recommended only for testing purposes, or in |
There was a problem hiding this comment.
Minor minor nitpick. Could you just remove the everything after the first sentence?
This is exactly what I need for Cumulus and this does not fit into one of these categories ;)
Relevant for #2416
Right now the
substrate-client-dbcrate has akvdb-rocksdbcompile-time feature. When enabled (which is the case by default), we use RocksDB. When disabled (which is the case for the in-browser experiments), we print a warning and use the memorydb instead.This pull request modifies this mechanic.
In both
substrate_service::config::Configurationandsubstrate_client_db::DatabaseSettingsit is now possible to pass either a path to a database, in which case it will be opened using RocksDB, or a custom implementation of thekvdb::KeyvalueDBtrait. The in-browser node would manually open a database (using the browser's local-storage) and pass it at initialization.The
kvdb-rocksdbfeature still exists insubstrate_client_db. If it is disabled, we return an error at initialization in the case of a path to a database. This is safer than just printing a warning. Another option could be to simply remove at compile-time the option to pass a path, rather than producing a runtime error, but from my experience putting items behind Cargo features usually becomes a source of annoying compilation errors.Thanks to this change, I also slightly cleaned up
substrate_client_db::Backend::new_test.