facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
28.73k stars 6.34k forks source link

Skip updating CFOptions if no value to update #13140

Open jaykorean opened 2 weeks ago

jaykorean commented 2 weeks ago

Summary

Similar to PR #8518 , skip updating the CFOptions when there's no real value to update. SetOptions() API won't update the CFOptions and no new OPTIONS file will be created if the values to be updated are the same as the current cf option values.

Test Plan

./options_test --gtest_filter="*SkipUpdatingOptionsWhenNoValuesToUpdate*"
pdillinger commented 1 week ago

This is still marked "draft." Are you looking for a preliminary review, or full review?

jaykorean commented 1 week ago

@pdillinger Right. This is not ready for the full review yet, but any suggestion / guidance would be appreciated as I'm still trying to fix the following problem.

Without making any changes to the OptionsTypeMap of MutableCFOptions, we won't be comparing the options for table factory properly. MutableCFOptionsAreEqual() will always return true for calls like SetOptions({{"block_based_table_factory", "{block_size=1234}"}})); even when the block_size for the block_based_table_factory is not 1234. I was able to find this because my first implementation broke tests like DBBloomFilterTest::MutatingRibbonFilterPolicy.

So I tried making changes to the OptionsTypeMap of the MutableCFOptions so that the comparison includes table_factory, block_based_table_factory and plain_table_factory. It seems to be able to detect the changes for table factory mutable options now, however, doing so broke VerifyTableFactory() and VerifyCFOptions() and caused the failure in several other tests and I am still looking into that.

One hacky way that I can think of is leaving OptionsTypeMap of the MutableCFOptions the way it is, and SetOptions() will always update the options if the new_options include table_factory options, but I am trying to think if there's a better way to do this.

If you have any suggestions or thoughts around this, please let me know.