Enhance RocksDB backend: complete interfaces and add VersionedStore tests

[rongma7]

This change is 

[ChenxingLi]

Reviewed 2 of 10 files at r1, all commit messages.
Reviewable status: 2 of 10 files reviewed, 8 unresolved discussions (waiting on @rongma7)

---

Cargo.toml line 9 at r1 (raw file):

thiserror = "1"
auto_impl = "1.2"
kvdb-rocksdb = { git = "https://github.com/Conflux-Chain/parity-common-rocksdb", rev = "3fcd845" }

Modify on version with kvdb-rocksdb <= 0.19, current branch is 0.22.

---

src/errors.rs line 43 at r1 (raw file):

    Custom(&'static str),
    #[error("An error occurred while interacting with the kvdb_rocksdb interface")]
    RocksDbError,

Why could a db error happen on decoding?

---

src/middlewares/versioned_flat_key_value/tests.rs line 1150 at r1 (raw file):

}

fn test_versioned_store<D: DatabaseTrait>(

Use the idea of modularity to reuse code. The general parts (such as DB read/write) and the non-general parts (such as DB initialization) should be separated, rather than placed in the same function.

The current incorrect mixing leads to the need to add new functions to the DatabaseTrait and prevents you from properly handling the deletion of test directories.

Make the test function accept a db: impl DatabaseTrait parameter, and then implement the database's preprocessing and postprocessing outside the function.

---

src/backends/table.rs line 26 at r1 (raw file):

    #[cfg(test)]
    fn iter_from_start(&self) -> Result<TableIter<T>>;

Just keep this interface for non-test mode.

---

src/backends/impls/kvdb_rocksdb.rs line 22 at r1 (raw file):

}

pub fn empty_rocksdb(num_cols: u32, path: &str) -> Result<kvdb_rocksdb::Database> {

Improper name, how do you know there is no existing database on this path?

open_database is enough.

---

src/backends/impls/kvdb_rocksdb.rs line 75 at r1 (raw file):

        use crate::backends::TableName;

        let db_path = "test_database";

The temporary dir should have a prefix __ like __test_database. After test, the temporary dir should be removed.

---

src/backends/mod.rs line 27 at r1 (raw file):

    /// Initializes an empty database.
    #[cfg(test)]
    fn empty_for_test() -> Result<Self>;

A trait should not have a conditional interface. Remove this interface from trait.

---

src/backends/table_name.rs line 31 at r1 (raw file):

impl TableName {
    pub fn max_index() -> u32 {

pub const fn

[rongma7]

Reviewable status: 0 of 10 files reviewed, 8 unresolved discussions (waiting on @ChenxingLi)

---

Cargo.toml line 9 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Modify on version with kvdb-rocksdb <= 0.19, current branch is 0.22.

Done.

---

src/errors.rs line 43 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Why could a db error happen on decoding?

Done.

---

src/backends/table.rs line 26 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Just keep this interface for non-test mode.

Done.

---

src/backends/table_name.rs line 31 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

pub const fn

Done.

---

src/backends/impls/kvdb_rocksdb.rs line 22 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Improper name, how do you know there is no existing database on this path?

open_database is enough.

Done.

---

src/backends/impls/kvdb_rocksdb.rs line 75 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

The temporary dir should have a prefix __ like __test_database. After test, the temporary dir should be removed.

Done.

---

src/middlewares/versioned_flat_key_value/tests.rs line 1150 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Use the idea of modularity to reuse code. The general parts (such as DB read/write) and the non-general parts (such as DB initialization) should be separated, rather than placed in the same function.

The current incorrect mixing leads to the need to add new functions to the DatabaseTrait and prevents you from properly handling the deletion of test directories.

Make the test function accept a db: impl DatabaseTrait parameter, and then implement the database's preprocessing and postprocessing outside the function.

Done.

---

src/backends/mod.rs line 27 at r1 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

A trait should not have a conditional interface. Remove this interface from trait.

Done.

[ChenxingLi]

Reviewed 1 of 10 files at r1, 5 of 9 files at r2, all commit messages.
Reviewable status: 6 of 10 files reviewed, 3 unresolved discussions (waiting on @rongma7)

---

src/backends/impls/kvdb_rocksdb.rs line 34 at r2 (raw file):

        {
            let owned = <T::Value>::decode(&v)
                .map_err(DatabaseError::DecodeError)?

Manually implement From<DecodeError>and From<io::Error> for StorageError.

Then you can use ? simply, instead of map_err.

---

src/backends/impls/kvdb_rocksdb.rs line 48 at r2 (raw file):

            .map(|kv| match kv {
                Ok((k, v)) => Ok((
                    Cow::Owned(<T::Key>::decode(&k)?.into_owned()),

If the input is Vec<u8>, use decode_owned instead. SmallVec can be converted to Vec.

Also check the other parts.

[rongma7]

Reviewable status: 6 of 10 files reviewed, 3 unresolved discussions (waiting on @ChenxingLi)

---

src/backends/impls/kvdb_rocksdb.rs line 34 at r2 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

Manually implement From<DecodeError>and From<io::Error> for StorageError.

Then you can use ? simply, instead of map_err.

Done.

---

src/backends/impls/kvdb_rocksdb.rs line 48 at r2 (raw file):

Previously, ChenxingLi (Chenxing Li) wrote…

If the input is Vec<u8>, use decode_owned instead. SmallVec can be converted to Vec.

Also check the other parts.

Done.

[ChenxingLi]

Reviewed 1 of 9 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rongma7)