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

feat: export persist_period_sec option and background_threads #448

Merged
merged 6 commits into from Jan 19, 2021

Conversation

developerfred
Copy link
Contributor

@developerfred developerfred commented Jul 21, 2020

exported functions:

close #447

Export PR on RocksDB
facebook/rocksdb#7168

@aleksuss
Copy link
Member

Thanks for the contributing. Overall looks good. But could you add some tests ?

@developerfred
Copy link
Contributor Author

@aleksuss Yes! I'll add now. thanks for the quick feedback

src/db_options.rs Outdated Show resolved Hide resolved
@developerfred developerfred force-pushed the issue/447-export-and-set branch 2 times, most recently from 2181a20 to 20773cf Compare July 21, 2020 16:15
@developerfred developerfred force-pushed the issue/447-export-and-set branch 3 times, most recently from 6f0a88a to e2518a1 Compare July 23, 2020 20:01
…cksdb#447

- [x] add test disable all threads on get_with_cache_and_bulkload_test
- [x] add test for stats_persist_period_sec
developerfred added a commit to developerfred/rocksdb that referenced this pull request Jul 23, 2020
facebook-github-bot pushed a commit to facebook/rocksdb that referenced this pull request Jul 28, 2020
Summary:
fixed
 - rust-rocksdb/rust-rocksdb#447
 -  rust-rocksdb/rust-rocksdb#448

Pull Request resolved: #7168

Reviewed By: cheng-chang

Differential Revision: D22736013

Pulled By: ajkr

fbshipit-source-id: fdd784aa75d26a367b9108b05ffdd94a2ae117d3
@ailisp
Copy link

ailisp commented Jul 28, 2020

@developerfred expose set_stats_persist_period_sec looks good, but disable background threads isn't done. Env::Default()->SetBackgroundThreads(0, Env::Priority::BOTTOM); // **need export** is a different function than max_background_jobs. Which you can tested by:

  1. launch program with lldb, place a break point before:
             assert_eq!(k.as_ref(), format!("{:0>4}", expected).as_bytes());
  1. run thread list

Without Env::Default()->SetBackgroundThreads you'll see 3+ threads. (set_stats_persist_period_sec will turn off one thread, but there's still two other background threads). Expected only 1 thread with Env::Default()->SetBackgroundThreads, for example, run lldb with this: https://github.com/nearprotocol/rocksdb/blob/no-thread-test/examples/compact_files_example.cc

@developerfred
Copy link
Contributor Author

@ailisp I will open a new pull request to export the Env::Default()->SetBackgroundThreads(0, Env::Priority::BOTTOM);

@developerfred
Copy link
Contributor Author

developerfred commented Jul 28, 2020

@ailisp I exported how rocksdb_env_set_background_threads_disable

developerfred added a commit to developerfred/rocksdb that referenced this pull request Jul 28, 2020
@ailisp
Copy link

ailisp commented Jul 28, 2020

@ailisp I exported how rocksdb_env_set_background_threads_disable

It might be good idea to follow their naming (lines before your change)

void rocksdb_env_set_high_priority_background_threads(rocksdb_env_t* env, int n) {
  env->rep->SetBackgroundThreads(n, Env::HIGH);
}

Also please also export Env::Default()->SetBackgroundThreads(0, Env::Priority::LOW)? Thanks!

@developerfred
Copy link
Contributor Author

developerfred commented Jul 28, 2020

@ailisp done 👍🏾 - rocksdb_env_set_bottom_priority_background_threads and rocksdb_env_set_low_priority_background_threads

facebook/rocksdb@19180d5

facebook-github-bot pushed a commit to facebook/rocksdb that referenced this pull request Jul 29, 2020
Summary:
- rust-rocksdb/rust-rocksdb#448

Pull Request resolved: #7191

Reviewed By: riversand963

Differential Revision: D22809066

Pulled By: ajkr

fbshipit-source-id: 036939f9a28cacc3f677c318d1aed97fe5f4f85e
@developerfred
Copy link
Contributor Author

developerfred commented Jul 29, 2020

@ailisp Bump done

2c3d7d7

@developerfred developerfred force-pushed the issue/447-export-and-set branch 2 times, most recently from 5e5a34b to e66b37b Compare July 30, 2020 02:59
developerfred added a commit to developerfred/rocksdb that referenced this pull request Jul 30, 2020
hunterlxt pushed a commit to hunterlxt/rust-rocksdb that referenced this pull request Jul 31, 2020
@ailisp
Copy link

ailisp commented Aug 6, 2020

@developerfred My bad in test, your PRs fully works, it just takes some time to wait rocksdb destroy other threads (wait some seconds and lldb attach there's only one thread). I'm very sorry of that. So no need to export Env::Default (ust-rocksdb/rust-rocksdb#448)

@developerfred
Copy link
Contributor Author

@ailisp ok! No problem!

@ailisp
Copy link

ailisp commented Aug 6, 2020

@developerfred We've successfully use your fork on our repo, Thank you! looking forward to next rust-rocksdb release, to upgrade your changes in c++ rocksdb and land this PR

@ailisp
Copy link

ailisp commented Jan 15, 2021

@stanislav-tkach Facebook has released rocksdb containing @developerfred 's change and it's been bumped in rust-rocksdb, let's merge this PR? (@developerfred Would you mind resolve conflict? I cannot push to your branch, if you don't have time I can fork from your fork, resolve conflict and make a new PR, then close this one)

@developerfred
Copy link
Contributor Author

@stanislav-tkach Facebook has released rocksdb containing @developerfred 's change and it's been bumped in rust-rocksdb, let's merge this PR? (@developerfred Would you mind resolve conflict? I cannot push to your branch, if you don't have time I can fork from your fork, resolve conflict and make a new PR, then close this one)

I solve this now, thanks for information and ping.

@aleksuss
Copy link
Member

@developerfred apply please suggestions by fmt and clippy.

@developerfred
Copy link
Contributor Author

ok @aleksuss

@aleksuss aleksuss merged commit ccf6ae3 into rust-rocksdb:master Jan 19, 2021
@ailisp
Copy link

ailisp commented Jan 19, 2021

Thank you @developerfred and @aleksuss !

codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
Summary:
fixed
 - rust-rocksdb/rust-rocksdb#447
 -  rust-rocksdb/rust-rocksdb#448

Pull Request resolved: facebook#7168

Reviewed By: cheng-chang

Differential Revision: D22736013

Pulled By: ajkr

fbshipit-source-id: fdd784aa75d26a367b9108b05ffdd94a2ae117d3
codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
Summary:
- rust-rocksdb/rust-rocksdb#448

Pull Request resolved: facebook#7191

Reviewed By: riversand963

Differential Revision: D22809066

Pulled By: ajkr

fbshipit-source-id: 036939f9a28cacc3f677c318d1aed97fe5f4f85e
romanz added a commit to romanz/rust-rocksdb that referenced this pull request Sep 10, 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.

export stats_persist_period_sec option and rocksdb_env_set_background_threads
4 participants