[
https://issues.apache.org/jira/browse/CASSANDRA-15254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17718325#comment-17718325
]
David Capwell commented on CASSANDRA-15254:
-------------------------------------------
the new patch and post from dev@ I feel is still trying to make this more
complex than needed, so took a moment to flesh out just leveraging Property and
YAML and avoiding the duplications of concerns... below adds everything we need
{code}
diff --git a/src/java/org/apache/cassandra/config/DatabaseDescriptor.java
b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java
index 21a55b7752..6434350cb2 100644
--- a/src/java/org/apache/cassandra/config/DatabaseDescriptor.java
+++ b/src/java/org/apache/cassandra/config/DatabaseDescriptor.java
@@ -26,7 +26,9 @@ import java.net.SocketException;
import java.net.UnknownHostException;
import java.nio.file.FileStore;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.Comparator;
import java.util.Enumeration;
import java.util.List;
@@ -43,6 +45,7 @@ import javax.annotation.Nullable;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.google.common.primitives.Longs;
@@ -63,6 +66,8 @@ import org.apache.cassandra.auth.IRoleManager;
import org.apache.cassandra.config.Config.CommitLogSync;
import org.apache.cassandra.config.Config.PaxosOnLinearizabilityViolation;
import org.apache.cassandra.config.Config.PaxosStatePurging;
+import org.apache.cassandra.config.Properties.Listener;
+import org.apache.cassandra.config.Properties.Validator;
import org.apache.cassandra.db.ConsistencyLevel;
import org.apache.cassandra.db.commitlog.AbstractCommitLogSegmentManager;
import org.apache.cassandra.db.commitlog.CommitLog;
@@ -90,6 +95,7 @@ import org.apache.cassandra.security.SSLFactory;
import org.apache.cassandra.service.CacheService.CacheType;
import org.apache.cassandra.service.paxos.Paxos;
import org.apache.cassandra.utils.FBUtilities;
+import org.yaml.snakeyaml.introspector.Property;
import static
org.apache.cassandra.config.CassandraRelevantProperties.ENABLE_SECONDARY_INDEX;
import static
org.apache.cassandra.config.CassandraRelevantProperties.HOST_REPLACE_TOKENS;
@@ -192,6 +198,98 @@ public class DatabaseDescriptor
private static long repairHistorySyncTimeoutSeconds;
+ public static ImmutableMap<String, Property> PROPERTIES = properties();
+
+ private static ImmutableMap<String, Property> properties()
+ {
+ ImmutableMap.Builder<String, Property> builder =
ImmutableMap.builder();
+ Loader loader = Properties.defaultLoader();
+ Map<String, Property> properties = loader.flatten(Config.class);
+ // only handling top-level replacements for now, previous logic was
only top level so not a regression
+ Map<String, Replacement> replacements =
Replacements.getNameReplacements(Config.class).get(Config.class);
+ if (replacements != null)
+ {
+ for (Replacement r : replacements.values())
+ {
+ Property latest = properties.get(r.newName);
+ assert latest != null : "Unable to find replacement new name:
" + r.newName;
+ Property conflict = properties.put(r.oldName,
r.toProperty(latest));
+ // some configs kept the same name, but changed the type, if
this is detected then rely on the replaced property
+ assert conflict == null || r.oldName.equals(r.newName) :
String.format("New property %s attempted to replace %s, but this property
already exists", latest.getName(), conflict.getName());
+ }
+ }
+ for (Map.Entry<String, Property> e : properties.entrySet())
+ {
+ String key = e.getKey();
+ Property prop = e.getValue();
+ List<Validator<?, ?>> validations = validations(key,
prop.getType());
+ List<Listener<?, ?>> listeners = listeners(key, prop.getType());
+ if (validations.isEmpty() && listeners.isEmpty())
+ {
+ builder.put(key, prop);
+ }
+ else
+ {
+ List<Listener<?, ?>> list = new ArrayList<>(listeners.size() +
1);
+ if (!validations.isEmpty())
+ list.add(new Properties.CompositeListener(validations));
+ list.addAll(listeners);
+ Listener<?, ?> listener = new
Properties.CompositeListener(list);
+ Properties.ListenableProperty<?, ?> listenProp = new
Properties.ListenableProperty(prop, listener);
+ builder.put(key, listenProp);
+ }
+ }
+
+ return builder.build();
+ }
+
+ private static List<Listener<?, ?>> listeners(String key, Class<?> type)
+ {
+ List<Listener<?, ?>> listeners = new ArrayList<>();
+ listeners.addAll(listeners(key));
+ listeners.addAll(listeners(type));
+ return listeners.isEmpty() ? Collections.emptyList() : listeners;
+ }
+
+ private static Collection<? extends Listener<?, ?>> listeners(Class<?>
type)
+ {
+ return Collections.emptyList();
+ }
+
+ private static Collection<? extends Listener<?, ?>> listeners(String key)
+ {
+ return Collections.emptyList();
+ }
+
+ private static List<Validator<?, ?>> validations(String key, Class<?> type)
+ {
+ List<Validator<?, ?>> validators = new ArrayList<>();
+ validators.addAll(validations(key));
+ validators.addAll(validations(type));
+ return validators.isEmpty() ? Collections.emptyList() : validators;
+ }
+
+ private static Collection<? extends Validator<?, ?>> validations(Class<?>
type)
+ {
+ return Collections.emptyList();
+ }
+
+ private static Collection<? extends Validator<?, ?>> validations(String
key)
+ {
+ switch (key)
+ {
+ case "concurrent_compactors":
+ return Arrays.asList((Validator<Config, Integer>)
DatabaseDescriptor::validateConcurrentCompactors);
+ }
+ return Collections.emptyList();
+ }
+
+ private static void validateConcurrentCompactors(int concurrent_compactors)
+ {
+ if (concurrent_compactors <= 0)
+ throw new ConfigurationException("concurrent_compactors should be
strictly greater than 0, but was " + conf.concurrent_compactors, false);
+ }
+
public static void daemonInitialization() throws ConfigurationException
{
daemonInitialization(DatabaseDescriptor::loadConfig);
@@ -711,8 +809,7 @@ public class DatabaseDescriptor
if (conf.concurrent_compactors == null)
conf.concurrent_compactors = Math.min(8, Math.max(2,
Math.min(FBUtilities.getAvailableProcessors(),
conf.data_file_directories.length)));
- if (conf.concurrent_compactors <= 0)
- throw new ConfigurationException("concurrent_compactors should be
strictly greater than 0, but was " + conf.concurrent_compactors, false);
+ validateConcurrentCompactors(conf.concurrent_compactors);
applyConcurrentValidations(conf);
applyRepairCommandPoolSize(conf);
diff --git a/src/java/org/apache/cassandra/config/ForwardingProperty.java
b/src/java/org/apache/cassandra/config/ForwardingProperty.java
index 5e7840e99c..28f27bb461 100644
--- a/src/java/org/apache/cassandra/config/ForwardingProperty.java
+++ b/src/java/org/apache/cassandra/config/ForwardingProperty.java
@@ -32,6 +32,11 @@ public class ForwardingProperty extends Property
{
private final Property delegate;
+ public ForwardingProperty(Property property)
+ {
+ this(property.getName(), property);
+ }
+
public ForwardingProperty(String name, Property property)
{
this(name, property.getType(), property);
diff --git a/src/java/org/apache/cassandra/config/Properties.java
b/src/java/org/apache/cassandra/config/Properties.java
index 79852d4af6..880eacea42 100644
--- a/src/java/org/apache/cassandra/config/Properties.java
+++ b/src/java/org/apache/cassandra/config/Properties.java
@@ -21,6 +21,7 @@ import java.lang.reflect.Constructor;
import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Collections;
+import java.util.List;
import java.util.Map;
import java.util.Queue;
@@ -193,4 +194,67 @@ public final class Properties
}
}
}
+
+ public interface Listener<A, B>
+ {
+ default void onPreSet(A object, B value) {}
+ default void onPostSet(A object, B value) {}
+ }
+
+ public static class CompositeListener<A, B> implements Listener<A, B>
+ {
+ private final List<? extends Listener<A, B>> listeners;
+
+ public CompositeListener(List<? extends Listener<A, B>> listeners)
+ {
+ this.listeners = listeners;
+ }
+
+ @Override
+ public void onPreSet(A object, B value)
+ {
+ for (Listener<A, B> l : listeners)
+ l.onPreSet(object, value);
+ }
+
+ @Override
+ public void onPostSet(A object, B value)
+ {
+ for (Listener<A, B> l : listeners)
+ l.onPostSet(object, value);
+ }
+ }
+
+ public interface Validator<A, B> extends Listener<A, B>
+ {
+ void validate(B value);
+ default void validate(A object, B value)
+ {
+ validate(value);
+ }
+
+ @Override
+ default void onPreSet(A object, B value)
+ {
+ validate(object, value);
+ }
+ }
+
+ public static class ListenableProperty<A, B> extends ForwardingProperty
+ {
+ private final Listener<A, B> listener;
+ public ListenableProperty(Property property, Listener<A, B> listener)
+ {
+ super(property);
+ this.listener = listener;
+ }
+
+ @Override
+ public void set(Object o, Object o1) throws Exception
+ {
+ listener.onPreSet((A) o, (B) o1);
+ super.set(o, o1);
+ listener.onPostSet((A) o, (B) o1);
+ }
+ }
}
diff --git a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
index 528accdb74..6294fcc46b 100644
--- a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
+++ b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
@@ -363,7 +363,7 @@ public class YamlConfigurationLoader implements
ConfigurationLoader
deprecationWarnings.add(result.getName());
}
- return new ForwardingProperty(result.getName(), result)
+ return new ForwardingProperty(result)
{
boolean allowsNull = result.getAnnotation(Nullable.class) !=
null;
diff --git a/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
b/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
index 7a4c39e647..2d5471a507 100644
--- a/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
+++ b/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
@@ -24,10 +24,7 @@ import com.google.common.collect.ImmutableMap;
import org.apache.cassandra.config.Config;
import org.apache.cassandra.config.DatabaseDescriptor;
-import org.apache.cassandra.config.Loader;
import org.apache.cassandra.config.Properties;
-import org.apache.cassandra.config.Replacement;
-import org.apache.cassandra.config.Replacements;
import org.apache.cassandra.db.DecoratedKey;
import org.apache.cassandra.db.marshal.UTF8Type;
import org.apache.cassandra.dht.LocalPartitioner;
@@ -91,21 +88,7 @@ final class SettingsTable extends AbstractVirtualTable
private static Map<String, Property> getProperties()
{
- Loader loader = Properties.defaultLoader();
- Map<String, Property> properties = loader.flatten(Config.class);
- // only handling top-level replacements for now, previous logic was
only top level so not a regression
- Map<String, Replacement> replacements =
Replacements.getNameReplacements(Config.class).get(Config.class);
- if (replacements != null)
- {
- for (Replacement r : replacements.values())
- {
- Property latest = properties.get(r.newName);
- assert latest != null : "Unable to find replacement new name:
" + r.newName;
- Property conflict = properties.put(r.oldName,
r.toProperty(latest));
- // some configs kept the same name, but changed the type, if
this is detected then rely on the replaced property
- assert conflict == null || r.oldName.equals(r.newName) :
String.format("New property %s attempted to replace %s, but this property
already exists", latest.getName(), conflict.getName());
- }
- }
+ Map<String, Property> properties = new
HashMap<>(DatabaseDescriptor.PROPERTIES);
for (Map.Entry<String, String> e :
BACKWARDS_COMPATABLE_NAMES.entrySet())
{
String oldName = e.getKey();
{code}
What is going on? We have a Map<String, Property>, so we can "decorate" the
property to add listener support and validation (which is implemented as a
pre-listener).... How do we solve the "how do we map the validators?" problem?
We push this to DatabaseDescriptor (which currently owns this logic)....
When working with JMX and Settings table we can use strings, which means that
YAML is able to do all the type mapping (rather than creating new mapping
framework), see
org.apache.cassandra.config.YamlConfigurationLoader#updateFromMap
This also makes it so if we wanted to we could add a new annotation to define
validations, as many configs have the same logic, so do we need to have
multiple checks?
{code}
@Validation(PositiveInt.class)
public volatile Integer concurrent_compactors;
{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: 4h 40m
> 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: [email protected]
For additional commands, e-mail: [email protected]
