Add Options::DisableExtraChecks, clarify force_consistency_checks #9363
Conversation
Summary: In response to facebook#9354, this PR adds a way for users to "opt out" of extra checks that can impact peak write performance, which currently only includes force_consistency_checks. I considered including some other options but did not see a db_bench performance difference. Also clarify in comment for force_consistency_checks that it can "slow down saturated writing." Test Plan: basic coverage in unit tests Using my perf test in facebook#9354 comment, I see force_consistency_checks=true -> 725360 ops/s force_consistency_checks=false -> 783072 ops/s
|
OldDefaults is not maintained and I would not trust it to be unless we had some automated way of validating it. Lack of PRs/issues suggests either people aren't too interested in using it or aren't sensitive to its inaccuracy. I'm more inclined to mark it as "unmaintained" than to fix this one case so long after the change. |
|
|
||
| DEFINE_bool(check_flush_compaction_key_order, | ||
| ROCKSDB_NAMESPACE::Options().check_flush_compaction_key_order, | ||
| "During flush or compaction, check whether keys inserted to " |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
include/rocksdb/options.h
Outdated
| // DEPRECATED: This function might be removed in a future release. | ||
| // In general, defaults are changed to suit broad interests. Opting | ||
| // out of a change on upgrade should be deliberate and considered | ||
| // rather than automatic and thoughtless. |
There was a problem hiding this comment.
Can we end the comment after "considered". The rest of sounds mean and condescending.
| // The function recovers options to the option as in version 4.6. | ||
| // Change to some default settings from an older version. | ||
| // NOT MAINTAINED: This function has not been and is not maintained. | ||
| // DEPRECATED: This function might be removed in a future release. |
There was a problem hiding this comment.
Personally, I am sad that this is not better maintained but know that it has not been and it would be hard to recreate. This function could be a good way to test (via db_bench or db_stress) whether or not options were causing performance or other regressions.
Having said that, I can see how this function would be very difficult to maintain. Perhaps instead there should be a section in HISTORY (or some clear markings there) to indicate what options were added or defaults were changed in a given release. This should allow better correlation of running tests/regressions of functionality between releases.
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: `ColumnFamilyOptions::OldDefaults` and `DBOptions::OldDefaults` now deprecated. Were previously overlooked with `Options::OldDefaults` in #9363 Pull Request resolved: #9594 Test Plan: comments only Reviewed By: jay-zhuang Differential Revision: D34318592 Pulled By: pdillinger fbshipit-source-id: 773c97a61e2a8290ae154f363dd61c1f35a9dd16
Summary: In response to #9354, this PR adds a way for users to "opt out"
of extra checks that can impact peak write performance, which
currently only includes force_consistency_checks. I considered including
some other options but did not see a db_bench performance difference.
Also clarify in comment for force_consistency_checks that it can "slow
down saturated writing."
Test Plan: basic coverage in unit tests
Using my perf test in #9354 comment, I see
force_consistency_checks=true -> 725360 ops/s
force_consistency_checks=false -> 783072 ops/s