From 8863012a19b722a995f01a56a49f4ef05594cc75 Mon Sep 17 00:00:00 2001 From: Oleksandr Anyshchenko Date: Tue, 31 Jul 2018 15:35:04 +0300 Subject: [PATCH 1/2] Methods `crate_cf` and `drop_cf` are immutable. --- src/db.rs | 36 +++++++++++++++++------------------- src/lib.rs | 3 ++- tests/test_column_family.rs | 4 ++-- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/db.rs b/src/db.rs index 78fd8c7..fb4e3d4 100644 --- a/src/db.rs +++ b/src/db.rs @@ -28,6 +28,7 @@ use std::path::Path; use std::ptr; use std::slice; use std::str; +use std::sync::{Arc, RwLock}; pub fn new_bloom_filter(bits: c_int) -> *mut ffi::rocksdb_filterpolicy_t { unsafe { ffi::rocksdb_filterpolicy_create_bloom(bits) } @@ -630,7 +631,7 @@ impl DB { } let db: *mut ffi::rocksdb_t; - let mut cf_map = BTreeMap::new(); + let cf_map = Arc::new(RwLock::new(BTreeMap::new())); if cfs.len() == 0 { unsafe { @@ -682,7 +683,7 @@ impl DB { } for (n, h) in cfs_v.iter().zip(cfhandles) { - cf_map.insert(n.name.clone(), ColumnFamily { inner: h }); + cf_map.write().unwrap().insert(n.name.clone(), ColumnFamily { inner: h }); } } @@ -838,7 +839,7 @@ impl DB { self.get_cf_opt(cf, key, &ReadOptions::default()) } - pub fn create_cf(&mut self, name: &str, opts: &Options) -> Result { + pub fn create_cf(&self, name: &str, opts: &Options) -> Result { let cname = match CString::new(name.as_bytes()) { Ok(c) => c, Err(_) => { @@ -856,31 +857,28 @@ impl DB { cname.as_ptr(), )); let cf = ColumnFamily { inner: cf_handler }; - self.cfs.insert(name.to_string(), cf); + self.cfs.write().unwrap().insert(name.to_string(), cf); cf }; Ok(cf) } - pub fn drop_cf(&mut self, name: &str) -> Result<(), Error> { - let cf = self.cfs.get(name); - if cf.is_none() { - return Err(Error::new( - format!("Invalid column family: {}", name).to_owned(), - )); - } - unsafe { - ffi_try!(ffi::rocksdb_drop_column_family( - self.inner, - cf.unwrap().inner, - )); + pub fn drop_cf(&self, name: &str) -> Result<(), Error> { + if let Some(cf) = self.cfs.write().unwrap().remove(name) { + unsafe { + ffi_try!(ffi::rocksdb_drop_column_family(self.inner, cf.inner,)); + } + Ok(()) + } else { + Err(Error::new( + format!("Invalid column family: {}", name).to_owned() + )) } - Ok(()) } /// Return the underlying column family handle. pub fn cf_handle(&self, name: &str) -> Option { - self.cfs.get(name).cloned() + self.cfs.read().unwrap().get(name).cloned() } pub fn iterator(&self, mode: IteratorMode) -> DBIterator { @@ -1207,7 +1205,7 @@ impl Drop for WriteBatch { impl Drop for DB { fn drop(&mut self) { unsafe { - for cf in self.cfs.values() { + for cf in self.cfs.read().unwrap().values() { ffi::rocksdb_column_family_handle_destroy(cf.inner); } ffi::rocksdb_close(self.inner); diff --git a/src/lib.rs b/src/lib.rs index b8193bf..fcac5f9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -72,13 +72,14 @@ use std::collections::BTreeMap; use std::error; use std::fmt; use std::path::PathBuf; +use std::sync::{Arc, RwLock}; /// A RocksDB database. /// /// See crate level documentation for a simple usage example. pub struct DB { inner: *mut ffi::rocksdb_t, - cfs: BTreeMap, + cfs: Arc>>, path: PathBuf, } diff --git a/tests/test_column_family.rs b/tests/test_column_family.rs index 97ac54f..c9993da 100644 --- a/tests/test_column_family.rs +++ b/tests/test_column_family.rs @@ -25,7 +25,7 @@ pub fn test_column_family() { let mut opts = Options::default(); opts.create_if_missing(true); opts.set_merge_operator("test operator", test_provided_merge, None); - let mut db = DB::open(&opts, path).unwrap(); + let db = DB::open(&opts, path).unwrap(); let opts = Options::default(); match db.create_cf("cf1", &opts) { Ok(_) => println!("cf1 created successfully"), @@ -81,7 +81,7 @@ pub fn test_column_family() { } // should b able to drop a cf { - let mut db = DB::open_cf(&Options::default(), path, &["cf1"]).unwrap(); + let db = DB::open_cf(&Options::default(), path, &["cf1"]).unwrap(); match db.drop_cf("cf1") { Ok(_) => println!("cf1 successfully dropped."), Err(e) => panic!("failed to drop column family: {}", e), From b7b456954a6536903dfe3c630e786a686c55459a Mon Sep 17 00:00:00 2001 From: Oleksandr Anyshchenko Date: Thu, 29 Nov 2018 16:49:52 +0200 Subject: [PATCH 2/2] Review changes --- src/db.rs | 30 ++++++++++++++++++------------ src/lib.rs | 2 +- tests/test_column_family.rs | 4 ++-- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/db.rs b/src/db.rs index c29d7fa..e00b5b3 100644 --- a/src/db.rs +++ b/src/db.rs @@ -384,7 +384,7 @@ impl DBRawIterator { /// if the iterator's seek position is ever moved by any of the seek commands or the /// ``.next()`` and ``.previous()`` methods as the underlying buffer may be reused /// for something else or freed entirely. - pub unsafe fn key_inner<'a>(&'a self) -> Option<&'a [u8]> { + pub unsafe fn key_inner(&self) -> Option<&[u8]> { if self.valid() { let mut key_len: size_t = 0; let key_len_ptr: *mut size_t = &mut key_len; @@ -408,7 +408,7 @@ impl DBRawIterator { /// if the iterator's seek position is ever moved by any of the seek commands or the /// ``.next()`` and ``.previous()`` methods as the underlying buffer may be reused /// for something else or freed entirely. - pub unsafe fn value_inner<'a>(&'a self) -> Option<&'a [u8]> { + pub unsafe fn value_inner(&self) -> Option<&[u8]> { if self.valid() { let mut val_len: size_t = 0; let val_len_ptr: *mut size_t = &mut val_len; @@ -452,7 +452,7 @@ impl DBIterator { mode: IteratorMode, ) -> Result { let mut rv = DBIterator { - raw: try!(DBRawIterator::new_cf(db, cf_handle, readopts)), + raw: DBRawIterator::new_cf(db, cf_handle, readopts)?, direction: Direction::Forward, // blown away by set_mode() just_seeked: false, }; @@ -686,7 +686,9 @@ impl DB { } for (n, h) in cfs_v.iter().zip(cfhandles) { - cf_map.write().unwrap().insert(n.name.clone(), ColumnFamily { inner: h }); + cf_map.write() + .map_err(|e| Error::new(e.to_string()))? + .insert(n.name.clone(), ColumnFamily { inner: h }); } } @@ -860,14 +862,16 @@ impl DB { cname.as_ptr(), )); let cf = ColumnFamily { inner: cf_handler }; - self.cfs.write().unwrap().insert(name.to_string(), cf); + self.cfs.write().map_err(|e| Error::new(e.to_string()))? + .insert(name.to_string(), cf); cf }; Ok(cf) } pub fn drop_cf(&self, name: &str) -> Result<(), Error> { - if let Some(cf) = self.cfs.write().unwrap().remove(name) { + if let Some(cf) = self.cfs.write().map_err(|e| Error::new(e.to_string()))? + .remove(name) { unsafe { ffi_try!(ffi::rocksdb_drop_column_family(self.inner, cf.inner,)); } @@ -881,7 +885,7 @@ impl DB { /// Return the underlying column family handle. pub fn cf_handle(&self, name: &str) -> Option { - self.cfs.read().unwrap().get(name).cloned() + self.cfs.read().ok()?.get(name).cloned() } pub fn iterator(&self, mode: IteratorMode) -> DBIterator { @@ -898,7 +902,7 @@ impl DB { DBIterator::new(self, &opts, mode) } - pub fn prefix_iterator<'a>(&self, prefix: &'a [u8]) -> DBIterator { + pub fn prefix_iterator(&self, prefix: &[u8]) -> DBIterator { let mut opts = ReadOptions::default(); opts.set_prefix_same_as_start(true); DBIterator::new(self, &opts, IteratorMode::From(prefix, Direction::Forward)) @@ -923,10 +927,10 @@ impl DB { DBIterator::new_cf(self, cf_handle, &opts, mode) } - pub fn prefix_iterator_cf<'a>( + pub fn prefix_iterator_cf( &self, cf_handle: ColumnFamily, - prefix: &'a [u8] + prefix: &[u8] ) -> Result { let mut opts = ReadOptions::default(); opts.set_prefix_same_as_start(true); @@ -1217,8 +1221,10 @@ impl Drop for WriteBatch { impl Drop for DB { fn drop(&mut self) { unsafe { - for cf in self.cfs.read().unwrap().values() { - ffi::rocksdb_column_family_handle_destroy(cf.inner); + if let Ok(cfs) = self.cfs.read() { + for cf in cfs.values() { + ffi::rocksdb_column_family_handle_destroy(cf.inner); + } } ffi::rocksdb_close(self.inner); } diff --git a/src/lib.rs b/src/lib.rs index 9534660..f090908 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -101,7 +101,7 @@ pub struct Error { impl Error { fn new(message: String) -> Error { - Error { message: message } + Error { message } } pub fn to_string(self) -> String { diff --git a/tests/test_column_family.rs b/tests/test_column_family.rs index ff47f89..59aa845 100644 --- a/tests/test_column_family.rs +++ b/tests/test_column_family.rs @@ -27,7 +27,7 @@ pub fn test_column_family() { let mut opts = Options::default(); opts.create_if_missing(true); opts.set_merge_operator("test operator", test_provided_merge, None); - let db = DB::open(&opts, path).unwrap(); + let db = DB::open(&opts, &n).unwrap(); let opts = Options::default(); match db.create_cf("cf1", &opts) { Ok(_db) => println!("cf1 created successfully"), @@ -83,7 +83,7 @@ pub fn test_column_family() { } // should b able to drop a cf { - let db = DB::open_cf(&Options::default(), path, &["cf1"]).unwrap(); + let db = DB::open_cf(&Options::default(), &n, &["cf1"]).unwrap(); match db.drop_cf("cf1") { Ok(_) => println!("cf1 successfully dropped."), Err(e) => panic!("failed to drop column family: {}", e),