[
https://issues.apache.org/jira/browse/CASSANDRA-15254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17705661#comment-17705661
]
Maxim Muzafarov commented on CASSANDRA-15254:
---------------------------------------------
cc [~dcapwell]
I had a productive discussion with David, so I would like to report valuable
notes and combine them with comments left by David in the pull requests. The
scope of this work described above remains unchanged.
The changes highlighted the problem we have with validating configuration
properties, which I'll describe below, and will schedule another round of
discussion on the dev list.
h2. Details for the scope
# A user interacts with Cassandra's internal configuration using API: JMX,
YAML file, and SettingsTable (under development). We need to perform the
following operations for each property regardless of which interface is used:
## validation (bounds checking, null value acceptance, etc.)
## type conversion (type -> string, or string -> type);
## setting a default value when a condition is met;
# The publicly available user interfaces such as JMX, file and SettingsTable
(YAML may not be suitable in all checks) should use the ConfigurationRegistry
(introduced by these changes) to access the Config instance to ensure that all
the necessary checks are performed.
# The DatabaseDescriptor (GuardrailsOptions) has methods to access the Config
instance (e.g. DatabaseDescriptor#getCompactionTombstoneWarningThreshold()).
These methods must remain unchanged for performance reasons and are used by
internal subsystems to read values directly.
h2. The property validation problem
h3. What we have
Currently, we are validating a configuration property in the following parts of
the code, which looks a bit messy:
- in a property type itself if it's not primitive, e.g.
DataStorageSpec#validateQuantity
- rarely in nested configuration classes e.g.
AuditLogOptions#validateCategories
- during the configuration load from yaml-file for null, and non-null, see
YamlConfigurationLoader.PropertiesChecker#check
- during applying the configuration, e.g. DatabaseDescriptor#applySimpleConfig
- in DatabaseDescriptor setter methods e.g.
DatabaseDescriptor#setDenylistMaxKeysTotal
- inside custom classes e.g. SSLFactory#validateSslContext
- rarely in JMX methods itself, e.g. StorageService#setRepairRpcTimeout
We also perform at least two different types of property validation:
- simple validation such as checking value bounds, checking null or not null,
or matching pattern etc. (e.g. conf.memtable_cleanup_threshold < 0.01f);
- validation of a property whose value depends on the values of other
properties, see DatabaseDescriptor#setDefaultKeyspaceRF (this is out of the
scope of this work);
*What we have. Example #1:*
{code:java}
// We perform three validations:
// - inside the type IntMebibytesBound;
// - after loading YAML check for @Nullable;
// - when applying the simple config;
// - and each time when JMX method is called;
public volatile DataStorageSpec.IntMebibytesBound repair_session_space = null;
private static void applySimpleConfig() {
...
if (conf.repair_session_space.toMebibytes() < 1)
throw new ConfigurationException("repair_session_space must be > 0, but
was " + conf.repair_session_space);
...
}
public static void setRepairSessionSpaceInMiB(int sizeInMiB) {
if (sizeInMiB < 1)
throw new ConfigurationException("Cannot set repair_session_space to "
+ sizeInMiB + " < 1 mebibyte");
...
}
{code}
*What we have. Example #2:*
{code:java}
// We perform validations:
// - inside the type LongMillisecondsBound;
// - when applying the simple config;
// - the LOWEST_ACCEPTED_TIMEOUT check for JMX is missed;
public volatile DurationSpec.LongMillisecondsBound read_request_timeout = new
DurationSpec.LongMillisecondsBound("5000ms");
static void checkForLowestAcceptedTimeouts(Config conf)
{
if(conf.read_request_timeout.toMilliseconds() <
LOWEST_ACCEPTED_TIMEOUT.toMilliseconds())
{
logInfo("read_request_timeout", conf.read_request_timeout,
LOWEST_ACCEPTED_TIMEOUT);
conf.read_request_timeout = new
DurationSpec.LongMillisecondsBound("10ms");
}
}
public static void setRangeRpcTimeout(long timeOutInMillis)
{
conf.range_request_timeout = new
DurationSpec.LongMillisecondsBound(timeOutInMillis);
}
{code}
h3. What to do
In order to use the same validation path for configuration properties that are
going to be changed through {{SettingsTable}}, we need to arrange a common
validation process for each property to rely on, so that the validation path
will be the same regardless of the public interface used.
In general, I'd like to avoid building a complex validation framework for
Cassandra's configuration, as the purpose of the project is not to maintain the
configuration itself, so the simpler and tiny the validation of the properties
will be, the easier the configuration itself will be. We might have the
following options for building the validation process, each of them has its own
pros and cons (including implementation difficulties).
h4. 1. New annotations
Add new annotations to build the property's validation rules (as it was
suggested by David)
@Max, @Min, @NotNull, @Size, @Nullable (already have this one), etc.
{code:java}
@NotNull @Min(5.0) @Max(16.0)
public volatile double phi_convict_threshold = 8.0;
{code}
An example of such an approach is the Dropwizard Configuration library (or
Hibernate, Spring)
[https://www.dropwizard.io/en/latest/manual/validation.html#annotations]
h4. 2. OR add validators to @Mutable
Add to the @Mutable the set (or single implementation) of validations it
performs, which is closer to what we have now.
As an alternative to having a single class for each constraint, we can have an
enumeration list that stores the same implementations.
{code:java}
public @interface Mutable {
Class<? extends ConfigurationConstraint<?>>[] constraints() default {};
}
public class NotNullConstraint implements ConfigurationConstraint<Object> {
public void validate(Object newValue) {
if (newValue == null)
throw new IllegalArgumentException("Value cannot be null");
}
}
public class PositiveConstraint implements ConfigurationConstraint<Object> {
public void validate(Object newValue) {
if (newValue instanceof Number && ((Number) newValue).intValue() <= 0)
throw new IllegalArgumentException("Value must be positive");
}
}
@Mutable(constraints = { NotNullConstraint.class, PositiveConstraint.class })
public volatile Integer concurrent_compactors;
{code}
Something similar is performed for Custom Constraints in Hibernate.
[https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#section-constraint-validator]
h4. 3. OR enforce setter method names
Enforce the setters method names to match the corresponding property name. This
will allow us to call the setter method with reflection, and the method itself
will do all the validation we need.
{code:java}
public volatile int key_cache_keys_to_save = Integer.MAX_VALUE;
public static void setKeyCacheKeysToSave(int keyCacheKeysToSave)
{
conf.key_cache_keys_to_save = keyCacheKeysToSave;
}
{code}
> Allow UPDATE on settings virtual table to change running configurations
> -----------------------------------------------------------------------
>
> Key: CASSANDRA-15254
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15254
> Project: Cassandra
> Issue Type: New Feature
> Components: Feature/Virtual Tables
> Reporter: Chris Lohfink
> Assignee: Maxim Muzafarov
> Priority: Normal
> Fix For: 5.x
>
> Attachments: Configuration Registry Diagram.png
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Allow using UPDATE on the system_views.settings virtual table to update
> configs at runtime for the equivalent of the dispersed JMX
> attributes/operations.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
