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

Remove disableDataSync option #1859

Closed
wants to merge 5 commits into from

Conversation

sagar0
Copy link
Contributor

@sagar0 sagar0 commented Feb 10, 2017

Remove disableDataSync, and another similarly named disable_data_sync options.
This is being done to simplify options, and also because the performance gains of this feature can be achieved by other methods.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

Nice!

Please update HISTORY.md for this API change.

OptionVerificationType::kNormal, false, 0}},
{"disable_data_sync", // for compatibility
{offsetof(struct DBOptions, disableDataSync), OptionType::kBoolean,
OptionVerificationType::kNormal, false, 0}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you still leave the two as OptionVerificationType::kDeprecated?

@facebook-github-bot
Copy link
Contributor

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

@sagar0
Copy link
Contributor Author

sagar0 commented Feb 10, 2017

Seems like I messed up my merge. Let me try again.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@sagar0
Copy link
Contributor Author

sagar0 commented Feb 11, 2017

I think I fixed the bad merges now :)

@siying
Copy link
Contributor

siying commented Feb 13, 2017

Awesome! Go to go!

@facebook-github-bot
Copy link
Contributor

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

@ajkr
Copy link
Contributor

ajkr commented Mar 11, 2017

Can you remove uses of disableDataSync in fbcode? In the future consider just marking the option as deprecated, it can save some migration effort compared to breaking API changes :p.

@ajkr ajkr reopened this Mar 11, 2017
@sagar0
Copy link
Contributor Author

sagar0 commented Mar 11, 2017

Oh, didn't know I had to change something in fbcode as well. Sure, I'll try fixing it on Monday.

@ajkr
Copy link
Contributor

ajkr commented Mar 12, 2017

Thanks!

@ajkr ajkr closed this Mar 12, 2017
@jgraettinger
Copy link

This is being done ... because the performance gains of this feature can be achieved by other methods.

Would you please link or comment here as to what other methods you're referring to? Also, FYI this option is still referenced from the basic operations and RocksDB-Tuning-Guide wiki pages. Thanks!

@sagar0
Copy link
Contributor Author

sagar0 commented Mar 20, 2017

@jgraettinger Thanks for pointing out the referenced pages; I removed disableDataSync references from them, and also made sure that the wiki does not contain any more references (using google site search).

Would you please link or comment here as to what other methods you're referring to?

Having more flush and compaction threads is the way to go instead of continuing to use disableDataSync option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants