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

Validate compaction strategy options #13956

Merged
merged 16 commits into from
Sep 14, 2023
Merged

Validate compaction strategy options #13956

merged 16 commits into from
Sep 14, 2023

Conversation

Deexie
Copy link
Contributor

@Deexie Deexie commented May 19, 2023 •
edited

When a column family's schema is changed new compaction
strategy type may be applied.

To make sure that it will behave as expected, compaction
strategy need to contain only the allowed options and values.
Methods throwing exception on invalid options are added.

Fixes: #2336.

@Deexie
Copy link
Contributor Author

Deexie commented May 19, 2023

In draft since I am not sure if some options should be checked/allowed. Marked the places with comments above.

@Deexie
Copy link
Contributor Author

Deexie commented May 19, 2023

@bhalevy @raphaelsc review please

@mykaul
Copy link
Contributor

mykaul commented May 28, 2023

@bhalevy , @raphaelsc - ping for review (this is part of the quality improvement effort, so will be great to make progress on this)

bhalevy
bhalevy reviewed May 28, 2023
View reviewed changes
compaction/time_window_compaction_strategy.cc Outdated
return window_unit;
}

int time_window_compaction_strategy_options::validate_compaction_window_size(const std::map<sstring, sstring>& options) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's possible to pass a mutable std::map<sstring, sstring>& options
and to erase the relevant option once it's validated
so we can check at the end if there are unrecognized options that are left.
It's possible that due to a typo, the user means to set some options but the code doesn't recognize it and it might go unnoticed.
(I guess the "nested" size_tiered options will need to handled too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "nested" options are checked in the following commits. The whole validation has the structure:

  • validate_* helpers validate a single option
  • *_compaction_strategy_options::validate validate options specific for the respective strategy (and use ^)
  • compaction_strategy_impl::validate_options validates options that are common for each strategy type
  • *_compaction_strategy::validate_options validate "nested" options for each strategy (and use ^ and ^^)

bhalevy
bhalevy reviewed May 28, 2023
View reviewed changes
compaction/compaction_strategy.cc Outdated
throw exceptions::syntax_exception(sstring("Invalid long value ") + tmp_value.value() + " for " + TOMBSTONE_COMPACTION_INTERVAL_OPTION);
}

if (interval <= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

why not check tombstone_compaction_interval?
can it become negative due to wrap around?

@Deexie Deexie force-pushed the validate-compaction-strategy-options branch from 09a8103 to 5704326 Compare May 30, 2023 13:46
@Deexie
Copy link
Contributor Author

Deexie commented May 30, 2023 •
edited

  • rebase on size_tiered_compaction_strategy_options: make copy and move-able and time_window_compaction_strategy_options: make copy and move-able (and master)
  • added versions of validate_* with second argument (unchecked_options)
  • move single option checks from size_tiered_compaction_strategy_options::validate to the appropriate validate_*
NOTE: this change causes size_tiered_compaction_strategy_options constructor 
to throw in cases where it didn't before. More precisely when min_sstables_size
or cold_reads_to_omit has an illegal value. I think it's reasonable as it's invalid
anyway and since time_window_compaction_strategy_options acts similarly. 
Better to be verified though ;)
  • modified catch clause and the exception thrown inside it in compaction_strategy_impl::validate_options
  • should -> must; correct typos; deleted unnecessary empty lines
  • divide commit adding compaction_strategy_impl::validate_options into parts:
    • split compaction_strategy_impl constructor into validate_* methods
    • add validate common compaction strategy options
  • add breaks to switch-case in compaction_strategy_impl::validate_options_for_strategy_type
  • add compaction: unify exception messages commit

bhalevy
bhalevy reviewed May 30, 2023
View reviewed changes
Copy link
Member

@bhalevy bhalevy left a comment

Choose a reason for hiding this comment

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

LGTM overall.
I had one question regarding format

@Deexie Deexie force-pushed the validate-compaction-strategy-options branch from 5704326 to 460b020 Compare May 30, 2023 14:19
@Deexie
Copy link
Contributor Author

Deexie commented May 30, 2023

  • format -> fmt::format

@bhalevy
Copy link
Member

bhalevy commented May 31, 2023

To summarize our 1:1 conversation regarding valid, yet unsupported options, we need to:

  1. Double check that the options are valid on C* (it is presumed we inherited them from origin)
  2. Otherwise, fix the documentation
  3. For valid C* options that we don't support, just log a warning that the option is unsupported but don't return an error (so not to create a regression)

@Deexie Deexie force-pushed the validate-compaction-strategy-options branch from 460b020 to 96b551c Compare June 1, 2023 17:43
@Deexie
Copy link
Contributor Author

Deexie commented Jun 1, 2023

  • warn about "unsafe_aggressive_sstable_expiration" which is supported by cassandra but not by scylla
  • check options which are common for each compaction strategy type in compaction_strategy_impl::validate_options_for_strategy_type
  • add compaction_strategy_impl::validate_min_max_threshold

@Deexie
Copy link
Contributor Author

Deexie commented Sep 5, 2023

  • rebase

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/3486/

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/3528/

@kostja
Copy link
Contributor

kostja commented Sep 11, 2023

It would be lovely to get this moving, since currently it is affecting CI-stability.

bhalevy and others added 16 commits September 13, 2023 16:59
@bhalevy @Deexie
size_tiered_compaction_strategy_options: make copy and move-able
c9475d6 
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
@bhalevy @Deexie
time_window_compaction_strategy_options: make copy and move-able
e5cf6f0 
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
@Deexie
compaction: add validate method to compaction_strategy_options
a01dd13 
Add temporarily empty validate method to compaction_strategy_options.
The method will validate the options and help determining whether
only the allowed options were set.
@Deexie
compaction: split time_window_compaction_strategy_options construtor
c8c3c0e 
Split time_window_compaction_strategy_options constructor into
functions that will be reused for validation.
@Deexie
compaction: use helpers in validate_* functions
e3d8f71 
To be consistent with other compaction_strategy_options,
time_window_compaction_strategy_options uses compaction_strategy_impl::get_value
and cql3::statements::property_definitions::to_long helpers for
parsing.
@Deexie
compaction: make compaction strategy keys static constexpr
702c19f
@Deexie
compaction: split size_tiered_compaction_strategy_options constructor
50c1bb5 
Split size_tiered_compaction_strategy_options constructor into
methods that will be reused for validation.

Add additional checks providing that options' values are legal.
@Deexie
compaction: add method to validate min and max threshold
84fd90e 
Add compaction_strategy_impl::validate_min_max_threshold method
that will be used to validate min and max threshold values
for different compaction methods.
@Deexie
compaction: validate time_window_compaction_strategy specific options
7e5b6ea
@Deexie
compaction: validate size_tiered_compaction_strategy specific options
5c72bcd
@Deexie
compaction: split compaction_strategy_impl constructor
a2e6081 
Split compaction_strategy_impl constructor into methods that will
be reused for validation.

Add additional checks providing that options' values are legal.
@Deexie
compaction: validate common compaction strategy options
0ed39af 
Add compaction_strategy_impl::validate_options to validate common
compaction strategy options.
@Deexie
compaction: validate options used in different compaction strategies
44744d6 
For each compaction strategy, validate whether options values are valid.
@Deexie
compaction: cql3: validate options in check_restricted_table_properties
ac08b57 
Check whether valid compaction strategy options are set for the given
strategy type in check_restricted_table_properties.
@Deexie
compaction: unify exception messages
53ecc29 
Use fmt::format in exception messages in all methods validating
compaction strategies.
@Deexie
test: add test for compaction strategy validation
14598fd
@Deexie Deexie force-pushed the validate-compaction-strategy-options branch from 18f5b84 to 14598fd Compare September 13, 2023 15:12
@Deexie
Copy link
Contributor Author

Deexie commented Sep 13, 2023

  • rebase

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/3655/

@bhalevy
Copy link
Member

bhalevy commented Sep 14, 2023

CI state ABORTED - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/3314/

Looks unrelated. test_fencing timing out in debug mode.
I respinned it.

@scylladb-promoter
Copy link
Contributor

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/3668/

@scylladb-promoter
Copy link
Contributor

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/3674/

@Deexie
Copy link
Contributor Author

Deexie commented Sep 14, 2023

@scylladb/scylla-maint please merge

@scylladb-promoter scylladb-promoter merged commit 0564d00 into scylladb:master Sep 14, 2023
3 checks passed
@fruch fruch mentioned this pull request Oct 3, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphaelsc raphaelsc raphaelsc approved these changes

@bhalevy bhalevy bhalevy approved these changes

@nyh nyh Awaiting requested review from nyh nyh is a code owner

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

Successfully merging this pull request may close these issues.

Validate compaction strategy option values
7 participants
@Deexie @mykaul @bhalevy @scylladb-promoter @raphaelsc @nyh @kostja