Validate compaction strategy option values

[raphaelsc]

Compaction strategy option value should be validated when either creating or altering a column family's schema. A bad value can lead the strategy to behave very badly.

[tzach]

@raphaelsc can you provide a list of valid values?

[bhalevy]

@raphaelsc can you provide a list of valid values?

@tzach I sent a patch to the mailing list validating:

min_threshold > 2
max_threshold >= min_threshold

On top of that, we should probably have per-compaction-strategy validation to CS-specific options / semantics.

[bhalevy]

@Deexie can you please pick this up?
We've hit an issue in the field where the user got themselves into trouble by mixing configuration options for different compaction strategies.

[bhalevy]

What we want is each compaction strategy to validate the <key, value> map of options to make sure it recognizes all keys and also validate the values if needed.

[Deexie]

What we want is each compaction strategy to validate the <key, value> map of options to make sure it recognizes all keys

LCS and TWCS have an attribute: size_tiered_compaction_strategy_options _stcs_options. With these options size_tiered_compaction_strategy is temporarily created in some methods. Should options for aforementioned strategies be allowed to contain STCS-specific fields?

and also validate the values if needed.

Are the following constraints exhaustive and strict enough?

Common options:

• tombstone_threshold >= 0.0 && tombstone_threshold <= 1.0
• tombstone_compaction_interval > 0s

Size Tiered Compaction Strategy:

• bucket_low <= bucket_high
• bucket_high > 1.0
• bucket_low > 0.0 && bucket_low < 1.0
• min_sstable_size >= 0
• min_threshold < max_threshold
• min_threshold > 2

Leveled Compaction Strategy:

• sstable_size_in_mb > 0

For Time Window Compaction Strategy checks are already done.

(Docs of all strategies and their options: https://github.com/scylladb/scylladb/blob/master/docs/cql/compaction.rst)

Do we want to do the checks for Date-tiered Compaction Strategy (it's deprecated)?

[bhalevy]

@raphaelsc ☝️

[bhalevy]

ping @raphaelsc please confirm #2336 (comment)

[raphaelsc]

@Deexie

please check check_restricted_table_properties() in cql3/statements/create_table_statements.cc. It currently performs some validation, and we should call strategy-specific validation from there.

All strategies inherit common strategy options (tombstone_threshold, etc), see: compaction_strategy_impl in compaction/compaction_strategy_impl.hh .

I suggest adding a validate(options) method to compaction_strategy_impl class. the options can be retrieved via schema->compaction_strategy_options().

Then we'd do the same for size_tiered_compaction_strategy. Add a validate(options) method to it, which in turn calls compaction_strategy_impl::validate(options), and performs validation on STCS options.

The same logic applies to LCS and TWCS, where both inherit STCS options.

DTCS is deprecated, we can just ignore it.

[bhalevy]

@Deexie ping

[avikivity]

Only affects invalid DDL input, not backporting.