open database for read only #130

mypmc · 2017-09-19T04:49:34Z

No description provided.

siddontang · 2017-09-19T12:01:22Z

src/rocksdb.rs

+        Self::open_cf_for_read_only(opts, path, vec![], error_if_log_file_exist)
+    }
+
+    pub fn open_cf_for_read_only(


seem we can use most of the same codes for open and open read-only?

Thanks for your reply.
You mean "need more cleanup" ?
I'll try it.

RFC:

I think open_cf arguments should be

pub fn open_cf( opts: DBOptions, path: &str, cfs: Vec<(&str, ColumnFamilyOptions)>, ) -> Result<DB, String> { // ... }

not

pub fn open_cf( opts: DBOptions, path: &str, cfs: Vec<&str>, cf_opts: Vec<ColumnFamilyOptions>, ) -> Result<DB, String> { // ... }

The later is error prone, so I want to fix this.
Do you have any objections to this change?

I prefer not using tuple but a ColumnFamilyDescriptor like RocksDB. But this change may break the compatibility but can make the open function more elegant.

/cc @BusyJay @zhangjinpeng1987

Like this?

pub fn open_cf<T: Into<ColumnFamilyDescriptor>>( opts: DBOptions, path: &str, cfds: Vec<T>, ) -> Result<DB, String> { // ... }

I prefer &[T] :-)

Shall we just add a read_only parameter for open_cf?

I've add a method open_cf_internal,
every other open method invoke this.

fn open_cf_internal<'a, T>( opts: DBOptions, path: &str, cfds: Vec<T>, // if none, open for read write mode. // otherwise, open for read only. error_if_log_file_exist: Option<bool>, ) -> Result<DB, String> where T: Into<ColumnFamilyDescriptor<'a>>, { // ... }

I'm trying to change Vec<T> to &[T] now.

@siddontang @zhangjinpeng1987
Changing Vec<T> to &[T] may need to change type of DB
because of ColumnFamilyOptions's implementation of Clone and lifetime.

I think Vec<T> is better.

This change breaks API, but makes code more clean and readable too.

mypmc · 2017-09-22T03:50:22Z

I found that test added in 1742f18 (test_block_based_options) sometimes failed on my local machine, regardless of this PR.

any idea?

siddontang · 2017-09-22T06:02:34Z

Cool @feb29

But I think you should split this PR into two PRs, first is to refactor open function, the other is to support read only.

siddontang · 2017-09-22T06:03:03Z

@feb29

What is the failure output?

mypmc · 2017-09-22T06:50:53Z

About splitting into two PRs, I got it.

Test fails at this assertion .
I've added a message "TickerType::ReadAmpEstimateUsefulBytes is 0".

Running test on master branch by RUST_BACKTRACE=full cargo test -- --nocapture

output:

failures:

---- test_rocksdb_options::test_block_based_options stdout ----
	thread 'test_rocksdb_options::test_block_based_options' panicked at 'assertion failed: `(left != right)`
  left: `0`,
 right: `0`: TickerType::ReadAmpEstimateUsefulBytes is 0', tests/test_rocksdb_options.rs:589:4
stack backtrace:
   0:        0x1087c7883 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::h53146987acc82352
                               at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:        0x1087c3eb0 - std::sys_common::backtrace::_print::h9fa78c974a2ee44c
                               at src/libstd/sys_common/backtrace.rs:71
   2:        0x1087ca863 - std::panicking::default_hook::{{closure}}::haef816838063b9ba
                               at src/libstd/sys_common/backtrace.rs:60
                               at src/libstd/panicking.rs:381
   3:        0x1087ca398 - std::panicking::default_hook::hcc910ee5a6164755
                               at src/libstd/panicking.rs:391
   4:        0x1087cad72 - std::panicking::begin_panic::hebc1b3c9e3ddd291
                               at src/libstd/panicking.rs:577
   5:        0x1087cabd4 - std::panicking::begin_panic::hd4fcfecbdb093b17
                               at src/libstd/panicking.rs:538
   6:        0x1087cab22 - std::panicking::try::do_call::h053be7a0e9f0fc06
                               at src/libstd/panicking.rs:522
   7:        0x1086c73ee - test::test_rocksdb_options::test_block_based_options::hca0c75cd42a2df8c
                               at tests/test_rocksdb_options.rs:589
   8:        0x10874d351 - <F as test::FnBox<T>>::call_box::hfa2ff38dcb4e05e3
                               at src/libtest/lib.rs:1480
                               at /Users/travis/build/rust-lang/rust/src/libcore/ops/function.rs:223
                               at src/libtest/lib.rs:141
   9:        0x1087d79dc - panic_unwind::dwarf::eh::read_encoded_pointer::h2d6187d22092e042
                               at src/libpanic_unwind/lib.rs:99
  10:        0x10873e388 - std::sys_common::backtrace::__rust_begin_short_backtrace::h3e768cfa9215c721
                               at /Users/travis/build/rust-lang/rust/src/libstd/panicking.rs:459
                               at /Users/travis/build/rust-lang/rust/src/libstd/panic.rs:361
                               at src/libtest/lib.rs:1419
                               at /Users/travis/build/rust-lang/rust/src/libstd/sys_common/backtrace.rs:136
  11:        0x10873f207 - std::panicking::try::do_call::hec1329aeb27eec8b
                               at /Users/travis/build/rust-lang/rust/src/libstd/thread/mod.rs:400
                               at /Users/travis/build/rust-lang/rust/src/libstd/panic.rs:296
                               at /Users/travis/build/rust-lang/rust/src/libstd/panicking.rs:480
  12:        0x1087d79dc - panic_unwind::dwarf::eh::read_encoded_pointer::h2d6187d22092e042
                               at src/libpanic_unwind/lib.rs:99
  13:        0x108747063 - <F as alloc::boxed::FnBox<A>>::call_box::hbaa0460cc6757097
                               at /Users/travis/build/rust-lang/rust/src/libstd/panicking.rs:459
                               at /Users/travis/build/rust-lang/rust/src/libstd/panic.rs:361
                               at /Users/travis/build/rust-lang/rust/src/libstd/thread/mod.rs:399
                               at /Users/travis/build/rust-lang/rust/src/liballoc/boxed.rs:728
  14:        0x1087c9dab - std::sys::imp::thread::Thread::new::thread_start::haf898c1a45f09126
                               at /Users/travis/build/rust-lang/rust/src/liballoc/boxed.rs:738
                               at src/libstd/sys_common/thread.rs:24
                               at src/libstd/sys/unix/thread.rs:90
  15:        0x10954293a - _pthread_body
  16:        0x109542886 - _pthread_start


failures:
    test_rocksdb_options::test_block_based_options

test result: FAILED. 73 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

macOS Sierra (10.12.6)

siddontang · 2017-09-22T07:10:30Z

@huachaohuang

We should figure out why this value is 0.

open database for read only

d47cd46

siddontang reviewed Sep 19, 2017

View reviewed changes

Add type 'ColumnFamilyDescriptor' for 'open' related methods

b569d98

This change breaks API, but makes code more clean and readable too.

mypmc mentioned this pull request Sep 22, 2017

test test_block_based_options failed sometimes #137

Closed

mypmc closed this Sep 22, 2017

mypmc deleted the read-only branch September 25, 2017 05:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

open database for read only #130

open database for read only #130

mypmc commented Sep 19, 2017

siddontang Sep 19, 2017

mypmc Sep 20, 2017

mypmc Sep 20, 2017 •

edited

Loading

siddontang Sep 20, 2017

mypmc Sep 21, 2017 •

edited

Loading

siddontang Sep 21, 2017

zhangjinpeng87 Sep 21, 2017

mypmc Sep 21, 2017

mypmc Sep 21, 2017 •

edited

Loading

siddontang Sep 21, 2017

mypmc commented Sep 22, 2017

siddontang commented Sep 22, 2017

siddontang commented Sep 22, 2017

mypmc commented Sep 22, 2017

siddontang commented Sep 22, 2017

open database for read only #130

open database for read only #130

Conversation

mypmc commented Sep 19, 2017

siddontang Sep 19, 2017

Choose a reason for hiding this comment

mypmc Sep 20, 2017

Choose a reason for hiding this comment

mypmc Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

siddontang Sep 20, 2017

Choose a reason for hiding this comment

mypmc Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

siddontang Sep 21, 2017

Choose a reason for hiding this comment

zhangjinpeng87 Sep 21, 2017

Choose a reason for hiding this comment

mypmc Sep 21, 2017

Choose a reason for hiding this comment

mypmc Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

siddontang Sep 21, 2017

Choose a reason for hiding this comment

mypmc commented Sep 22, 2017

siddontang commented Sep 22, 2017

siddontang commented Sep 22, 2017

mypmc commented Sep 22, 2017

siddontang commented Sep 22, 2017

mypmc Sep 20, 2017 •

edited

Loading

mypmc Sep 21, 2017 •

edited

Loading

mypmc Sep 21, 2017 •

edited

Loading