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

Skip updating CFOptions if no value to update #13140

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jaykorean
Copy link
Contributor

@jaykorean jaykorean commented Nov 15, 2024

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*"

@jaykorean jaykorean changed the title Avoid regenerating OPTIONS if no value to update Avoid updating CFOptions if no value to update Nov 15, 2024
@jaykorean jaykorean changed the title Avoid updating CFOptions if no value to update Skip updating CFOptions if no value to update Nov 15, 2024
@jaykorean jaykorean force-pushed the no_updating_cf_opts_for_same_values branch from 693aa59 to 59aeaec Compare November 15, 2024 04:19
@jaykorean jaykorean force-pushed the no_updating_cf_opts_for_same_values branch from 59aeaec to ee8dd79 Compare November 15, 2024 04:22
@jaykorean jaykorean force-pushed the no_updating_cf_opts_for_same_values branch from 1ebdd14 to f2b0514 Compare November 15, 2024 21:55
@pdillinger
Copy link
Contributor

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

@jaykorean
Copy link
Contributor Author

jaykorean commented Nov 21, 2024

@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.

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.

3 participants