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.

Sign up for GitHub

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

Jump to bottom

dynamic setting of stats_dump_period_sec through SetDBOption() #2004

Closed
wants to merge 5 commits into from
Closed

dynamic setting of stats_dump_period_sec through SetDBOption() #2004

wants to merge 5 commits into from

Conversation

raza15
Copy link

@raza15 raza15 commented Mar 19, 2017

Resolved the following issue: #1930

@facebook-github-bot
Copy link
Contributor

@raza15 updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yiwu-arbug
yiwu-arbug suggested changes Mar 20, 2017
View reviewed changes
Copy link
Contributor

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! See comments inline.

HISTORY.md Outdated
@@ -44,6 +44,7 @@
* Support dynamically change `delayed_write_rate` and `max_total_wal_size` options via SetDBOptions().
* Introduce DB::DeleteRange for optimized deletion of large ranges of contiguous keys.
* Support dynamically change `delayed_write_rate` option via SetDBOptions().
* Support dynamically change `stats_dump_period_sec` option via SetDBOptions().
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the changelog to Unreleased section, which will be included in next release.

Copy link
Author

Choose a reason for hiding this comment

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

Done

util/db_options.h
@@ -94,6 +93,7 @@ struct MutableDBOptions {
uint64_t delayed_write_rate;
uint64_t max_total_wal_size;
uint64_t delete_obsolete_files_period_micros;
unsigned int stats_dump_period_sec;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the same field from ImmutableDBOptions.

Copy link
Author

@raza15 raza15 Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this is done. Please tell me if i'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. I don't know why I didn't see the change.

db/db_impl.cc Outdated
@@ -618,12 +618,12 @@ static void DumpMallocStats(std::string* stats) {
#endif // !ROCKSDB_LITE

void DBImpl::MaybeDumpStats() {
if (immutable_db_options_.stats_dump_period_sec == 0) return;
if (mutable_db_options_.stats_dump_period_sec == 0) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

mutable_db_options_ is guarded by DBImpl::mutex_ but MaybeDumpStats() is called without holding mutex. So there will be race condition on the option if another thread call SetDBOptions. You need to lock the mutex, get the value of stats_dump_period_sec, and unlock mutex to finish the rest of work here.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@facebook-github-bot
Copy link
Contributor

@raza15 updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@raza15 updated the pull request - view changes - changes since last import

@raza15
Copy link
Author

raza15 commented Mar 20, 2017

@yiwu-arbug please see the updated PR

yiwu-arbug
yiwu-arbug suggested changes Mar 20, 2017
View reviewed changes
Copy link
Contributor

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

Looking good! Please run make format to clang-format the code. And can you also include the java comment change? Thanks.

HISTORY.md Outdated
@@ -1,5 +1,6 @@
# Rocksdb Change Log
## Unreleased
* Support dynamically change `stats_dump_period_sec` option via SetDBOptions().
Copy link
Contributor

@yiwu-arbug yiwu-arbug Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Add "### Public API Change"

util/db_options.h
@@ -94,6 +93,7 @@ struct MutableDBOptions {
uint64_t delayed_write_rate;
uint64_t max_total_wal_size;
uint64_t delete_obsolete_files_period_micros;
unsigned int stats_dump_period_sec;
Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. I don't know why I didn't see the change.

@facebook-github-bot
Copy link
Contributor

@raza15 updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yiwu-arbug
Copy link
Contributor

Tests looking good. The only travis failure looks unrelated. @raza15 can you run make format and update the PR? Thanks!

@raza15
Copy link
Author

raza15 commented Mar 20, 2017 •
edited
Loading

@yiwu-arbug I am getting the following error when I run make format :

You didn't have clang-format-diff.py available in your computer!
You can download it by running: 
    curl --location http://goo.gl/iUW1u2 -o clang-format-diff.py
and move clang-format-diff.py to some directory within PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin

I have downloaded clang-format-diff.py and put it in my path directory. I have also installed clang. But I still get the error. Do you know how to solve this? (On both Ubuntu and mac OS X it is giving the same error)

@yiwu-arbug
Copy link
Contributor

No idea :( You can apply this command directly:
git diff -U0 $LAST_MASTER^ | $CLANG_FORMAT_DIFF -i -p 1

@facebook-github-bot
Copy link
Contributor

@raza15 updated the pull request - view changes - changes since last import

@raza15
Copy link
Author

raza15 commented Mar 21, 2017 •
edited
Loading

@yiwu-arbug updated the PR after successfully running make format. I'll start a new PR to update the instructions for running clang.

@facebook-github-bot
Copy link
Contributor

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yiwu-arbug yiwu-arbug mentioned this pull request Mar 21, 2017
Closed
@yiwu-arbug
Copy link
Contributor

Merging. Thank you again for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yiwu-arbug yiwu-arbug yiwu-arbug approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@raza15 @facebook-github-bot @yiwu-arbug