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

rocksdb: add Env related API #103

Closed
wants to merge 2 commits into from
Closed

rocksdb: add Env related API #103

wants to merge 2 commits into from

Conversation

choleraehyq
Copy link

PTAL @zhangjinpeng1987 @siddontang @andelf
Signed-off-by: Cholerae Hu [email protected]

Signed-off-by: Cholerae Hu <[email protected]>
@@ -949,6 +974,10 @@ impl Options {
crocksdb_ffi::crocksdb_options_set_allow_concurrent_memtable_write(self.inner, v);
}
}

pub fn set_env(&mut self, env: &Env) {

Choose a reason for hiding this comment

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

I think you should keep the Env in the Options. Otherwise, the Env may be destroyed quickly when dropping Env.

Copy link
Author

@choleraehyq choleraehyq Jul 20, 2017

Choose a reason for hiding this comment

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

We need to share Env among two or more db instances, but Options cannot be shared. We should keep Env's lifetime as the same to databases.

Copy link

@siddontang siddontang Jul 20, 2017

Choose a reason for hiding this comment

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

It is bad to let the users manage the lifetime of Env. We should use DB outside only.

@@ -216,6 +217,13 @@ pub enum DBTableProperty {
CompressionName = 17,
}

#[repr(C)]
pub enum Priority {
Copy link
Member

Choose a reason for hiding this comment

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

ThreadPoolPriority


pub fn set_background_threads(&mut self, n: Priority) {
unsafe {
crocksdb_ffi::crocksdb_env_set_background_threads(self.inner, n);
Copy link
Member

Choose a reason for hiding this comment

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

Miss number parameter

Copy link
Member

Choose a reason for hiding this comment

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

You should also use crocksdb_env_set_high_priority_background_threads.

Signed-off-by: Cholerae Hu <[email protected]>
@choleraehyq
Copy link
Author

After discuss with jinpeng, we found out that thread pool is a singleton, we don't need to expose Env API. So close this PR.

@siddontang
Copy link

so, how do we share the same Env with multi rocksdbs?

@choleraehyq
Copy link
Author

We do not need to do this. All rocksdb will share one thread pool implicitly.

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.

3 participants