Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Deprecate "paritydb-experimental" CLI in favour or "paritydb" #10975

Merged
paritytech-processbot merged 2 commits into from
Mar 4, 2022
Merged

Deprecate "paritydb-experimental" CLI in favour or "paritydb" #10975

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b838ff2
Deprecate paritydb-experimental
arkpar Mar 3, 2022
b642443
Updated comment
arkpar Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions client/cli/src/arg_enums.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,10 @@ pub enum Database {
/// ParityDb. <https://github.com/paritytech/parity-db/>
ParityDb,
/// Detect whether there is an existing database. Use it, if there is, if not, create new
/// instance of paritydb
/// instance of RocksDb

@cheme cheme Mar 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it parity db ?
(that is what I got when testing the polkadot parity-db pr, actually I did observe an artifact of rocksdb but it was not used)

@arkpar arkpar Mar 3, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I though "Auto" is now the default CLI setting and it defaults to paritydb. But apparently "RocksDb" is still default.

Auto,
/// ParityDb. <https://github.com/paritytech/parity-db/>
ParityDbDeprecated,
}

impl std::str::FromStr for Database {
Expand All @@ -212,6 +214,8 @@ impl std::str::FromStr for Database {
if s.eq_ignore_ascii_case("rocksdb") {
Ok(Self::RocksDb)
} else if s.eq_ignore_ascii_case("paritydb-experimental") {
Ok(Self::ParityDbDeprecated)

@arkpar arkpar Mar 3, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function gets called multiple times, so can't just print a warning here.

@bkchr bkchr Mar 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could only log it once using OnceCell for example

@arkpar arkpar Mar 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say I like this idea. Introducing global variable and yet another dependency for a trivial thing.

} else if s.eq_ignore_ascii_case("paritydb") {
Ok(Self::ParityDb)
} else if s.eq_ignore_ascii_case("auto") {
Ok(Self::Auto)
Expand All @@ -224,7 +228,7 @@ impl std::str::FromStr for Database {
impl Database {
/// Returns all the variants of this enum to be shown in the cli.
pub fn variants() -> &'static [&'static str] {
&["rocksdb", "paritydb-experimental", "auto"]
&["rocksdb", "paritydb", "paritydb-experimental", "auto"]

@cheme cheme Mar 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove paritydb-experimental from the list (not sure if it is working).

@arkpar arkpar Mar 3, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work. Clap produces an error if an unknown variant is used.

}
}

Expand Down
7 changes: 7 additions & 0 deletions client/cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,13 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
Ok(match database {
Database::RocksDb => DatabaseSource::RocksDb { path: rocksdb_path, cache_size },
Database::ParityDb => DatabaseSource::ParityDb { path: paritydb_path },
Database::ParityDbDeprecated => {
eprintln!(

@arkpar arkpar Mar 3, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using log here is problematic because it is not initialised yet.

"WARNING: \"paritydb-experimental\" database setting is deprecated and will be removed in future releases. \
Please update your setup to use the new value: \"paritydb\"."
);
DatabaseSource::ParityDb { path: paritydb_path }
},
Database::Auto => DatabaseSource::Auto { paritydb_path, rocksdb_path, cache_size },
})
}
Expand Down