Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DB::open_cf_with_ttl method. #505

Merged
merged 5 commits into from Apr 18, 2021
Merged

Conversation

fdeantoni
Copy link
Contributor

Added the ability to open a database with column families and TTL. It uses method ffi::rocksdb_open_column_families_with_ttl.

Copy link
Member

@aleksuss aleksuss left a comment

Choose a reason for hiding this comment

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

Overall looks good 👍🏻

@@ -110,6 +110,25 @@ impl DB {
})
}

/// Opens the database with a Time to Live compaction filter and column family names.
Copy link
Member

Choose a reason for hiding this comment

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

Couple nitpicks.

  1. As I can see, you provide default options when create ColumnFamilyDescriptor. I guess it should be added to the function description for notifying user about this.
  2. Sometimes a necessity occurs to create a ColumnFamilyDescriptor with custom options. What do you think about adding extra function with custom options for ColumnFamilyDescriptors ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing this PR @aleksuss! I have pushed a new commit that contains the fixes you suggested. Let me know if this addresses what you were suggesting.

Copy link
Member

Choose a reason for hiding this comment

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

I mean to create a method like this:

pub fn open_cf_with_ttl_opt<P, I, N>(
         opts: &Options,
         path: P,
         cfs: I,
         ttl: Duration,
         cfs_opts: Options,
     ) -> Result<DB, Error>
     where
         P: AsRef<Path>,
         I: IntoIterator<Item = N>,
         N: AsRef<str>,
     {
         let cfs = cfs
             .into_iter()
             .map(|name| ColumnFamilyDescriptor::new(name.as_ref(), cfs_opts.clone()));

         DB::open_cf_descriptors_internal(opts, path, cfs, &AccessType::WithTTL { ttl })
}

Added comment clarifying default cf options will be used for
`open_cf_with_ttl` and added method `open_cf_descriptors_with_ttl`
to allow opening up of database with TTL and ColumnFamilyDescriptors.

/// Opens a database with the given database with a Time to Live compaction filter and
/// column family descriptors.
pub fn open_cf_descriptors_with_ttl<P, I>(
Copy link
Member

Choose a reason for hiding this comment

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

It seems this method is redundant.

src/db.rs Outdated
.into_iter()
.map(|name| ColumnFamilyDescriptor::new(name.as_ref(), Options::default()));

DB::open_cf_descriptors_with_ttl(opts, path, cfs, ttl)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DB::open_cf_descriptors_with_ttl(opts, path, cfs, ttl)
DB::open_cf_descriptors_internal(opts, path, cfs, &AccessType::WithTTL { ttl })

@@ -110,6 +110,25 @@ impl DB {
})
}

/// Opens the database with a Time to Live compaction filter and column family names.
Copy link
Member

Choose a reason for hiding this comment

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

I mean to create a method like this:

pub fn open_cf_with_ttl_opt<P, I, N>(
         opts: &Options,
         path: P,
         cfs: I,
         ttl: Duration,
         cfs_opts: Options,
     ) -> Result<DB, Error>
     where
         P: AsRef<Path>,
         I: IntoIterator<Item = N>,
         N: AsRef<str>,
     {
         let cfs = cfs
             .into_iter()
             .map(|name| ColumnFamilyDescriptor::new(name.as_ref(), cfs_opts.clone()));

         DB::open_cf_descriptors_internal(opts, path, cfs, &AccessType::WithTTL { ttl })
}

@fdeantoni
Copy link
Contributor Author

Thanks for the feedback @aleksuss. Just a question:

I actually patterned DB::open_cf_descriptors_with_ttl after the existing DB::open_cf_descriptors method. I made DB::open_cf_with_ttl() use DB::open_cf_descriptors_with_ttl() so that the existing test would exercise both methods.

If DB::open_cf_descriptors_with_ttl is redundant, perhaps DB::open_cf_descriptors should be marked deprecated with a new method like the one you describe above (but without the TTL)? Something like:

    /// Opens a database with the given database options and column family descriptors.
    pub fn open_cf_opt<P, I>(opts: &Options, path: P, cfs: I, cfs_opts: Options) -> Result<DB, Error>
    where
        P: AsRef<Path>,
        I: IntoIterator<Item = ColumnFamilyDescriptor>,
    {
         let cfs = cfs
             .into_iter()
             .map(|name| ColumnFamilyDescriptor::new(name.as_ref(), cfs_opts.clone()));

        DB::open_cf_descriptors_internal(opts, path, cfs, &AccessType::ReadWrite)
    }

Follow up question: Implementing things this way does take away the ability to create / re-open a DB with ColumnFamilies having different configurations. I'm assuming there may be use cases where a database may want to have different configurations per column family?

@aleksuss
Copy link
Member

I'm assuming there may be use cases where a database may want to have different configurations per column family?

It is exactly what I've meant. But I rethought and I think the better way to do it in another PR.

@aleksuss aleksuss merged commit 6534bf1 into rust-rocksdb:master Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants