You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2023/05/16 19:04:54 UTC
[GitHub] [cassandra] dcapwell commented on a diff in pull request #2334: CASSANDRA-15254 Support updates on the virtuabl table SettingsTable (use annotations for property updates validation)
dcapwell commented on code in PR #2334:
URL: https://github.com/apache/cassandra/pull/2334#discussion_r1195515796
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -204,6 +205,8 @@ public MemtableOptions()
// Limit the maximum depth of repair session merkle trees
@Deprecated
public volatile Integer repair_session_max_tree_depth = null;
+ @Mutable
+ @Validate(useClass = DATABASE_DESCRIPTOR_CLASS, useClassMethod = "validateRepairSessionSpace")
Review Comment:
do you do string because `Class<DatabaseDescriptor>` would cause a circular ref?
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -3613,14 +3618,28 @@ public static int getRepairSessionSpaceInMiB()
public static void setRepairSessionSpaceInMiB(int sizeInMiB)
{
- if (sizeInMiB < 1)
- throw new ConfigurationException("Cannot set repair_session_space to " + sizeInMiB +
- " < 1 mebibyte");
- else if (sizeInMiB > (int) (Runtime.getRuntime().maxMemory() / (4 * 1048576)))
- logger.warn("A repair_session_space of " + conf.repair_session_space +
- " is likely to cause heap pressure.");
+ setProperty(ConfigFields.REPAIR_SESSION_SPACE, sizeInMiB == -1 ? null : new DataStorageSpec.IntMebibytesBound(sizeInMiB));
+ }
- conf.repair_session_space = new DataStorageSpec.IntMebibytesBound(sizeInMiB);
+ public static DataStorageSpec.IntMebibytesBound validateRepairSessionSpace(Config conf, String name, DataStorageSpec.IntMebibytesBound newValue)
+ {
+ DataStorageSpec.IntMebibytesBound value0 = newValue == null ? new DataStorageSpec.IntMebibytesBound(Math.max(1, SPACE_UPPER_BOUND_MB)) : newValue;
Review Comment:
think this gets back to a convo we had on slack, `delete` to C* is `DELETE` statement, which you are not supporting in this ticket... so `null` is only possible in 2 cases
1) Config default
2) `repair_session_space: null` or `repair_session_space: ~` (`~` because yaml is fancy...)
since you can define `null` in yaml, we can't really rely on changing the default... so we do need to "delay" defaults in some cases... lame...
I don't like a `validate` method "changing" the value, but we need to keep that behavior else we cause a regression in the `repair_session_space: null` case...
which I guess means the validation model can't be
```
Object obj = get()
for (Validator v : validators)
v.validate(obj)
```
and must be
```
Object obj = get()
for (Validator v : validators)
obj = v.validate(obj)
```
The issue with this model is we need to have deterministic ordering... I don't know if your patch allows multiple `@Validate` but if we do allow in the future, we would need to make sure the order is well understood (prob definition order if possible?)... If you add a validation *after* the fact, then insertion order I guess makes sense... but with static init, this can be tricky...
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -576,14 +606,7 @@ else if (conf.disk_access_mode == Config.DiskAccessMode.mmap_index_only)
conf.repair_session_max_tree_depth = 20;
}
- if (conf.repair_session_space == null)
- conf.repair_session_space = new DataStorageSpec.IntMebibytesBound(Math.max(1, (int) (Runtime.getRuntime().maxMemory() / (16 * 1048576))));
-
- if (conf.repair_session_space.toMebibytes() < 1)
- throw new ConfigurationException("repair_session_space must be > 0, but was " + conf.repair_session_space);
- else if (conf.repair_session_space.toMebibytes() > (int) (Runtime.getRuntime().maxMemory() / (4 * 1048576)))
- logger.warn("A repair_session_space of " + conf.repair_session_space+ " mebibytes is likely to cause heap pressure");
-
+ conf.repair_session_space = validateRepairSessionSpace(conf, ConfigFields.REPAIR_SESSION_SPACE, conf.repair_session_space);
Review Comment:
with `@Validate` do you still need to do this here? This *should* be validated in yaml now right?
##########
src/java/org/apache/cassandra/config/Validate.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.config;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Validate annotation to be used on configuration fields to validate the value of the field with
+ * a custom validator class and its method.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.FIELD)
+public @interface Validate
+{
+ String useClass() default "";
Review Comment:
shouldn't class be required? what does `@Validate` mean?
##########
src/java/org/apache/cassandra/config/WithReplacementLoader.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.config;
+
+import java.util.Map;
+
+import org.yaml.snakeyaml.introspector.Property;
+
+public class WithReplacementLoader extends DefaultLoader
+{
+ @Override
+ public Map<String, Property> getProperties(Class<?> root)
+ {
+ Map<String, Property> properties = super.getProperties(root);
+ // 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(root);
Review Comment:
`Loader` is generic and doesn't (nor should it) really know about `Config`... If you want to add this, you should create a different method than `org.apache.cassandra.config.Replacements#getNameReplacements` which doesn't do `getReplacementsRecursive` and logically does `getReplacements` (without recursive)
That function was created before we went the `Loader` route, which meant we needed the map *before* snake yaml started parsing... since you are *during* parsing, we can lazy load w/e we need
##########
src/java/org/apache/cassandra/config/WithReplacementLoader.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.config;
+
+import java.util.Map;
+
+import org.yaml.snakeyaml.introspector.Property;
+
+public class WithReplacementLoader extends DefaultLoader
Review Comment:
I find that object decorating >>>> inherence. So rather than `extends`, take a `Loader` as input is best, so we can use a different loader and keep this property
```
Loader loader = new WithReplacementLoader(new JacksonLoader(objectMapper));
```
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -424,5 +428,140 @@ public void check() throws ConfigurationException
logger.warn("{} parameters have been deprecated. They have new names and/or value format; For more information, please refer to NEWS.txt", deprecationWarnings);
}
}
+
+ /**
+ * Creates a YAML instance based on Cassandra's custom configuration classes and types. Yaml factory here is used
+ * to {@link org.yaml.snakeyaml.Yaml#dumpAs} and {@link org.yaml.snakeyaml.Yaml#load(String)} given object or
+ * string respectively, that in turn is used to serialize and deserialize configuration properties.
+ */
+ public static class YamlFactory
+ {
+ private static final List<TypeDescription> scalarCassandraTypes = new ArrayList<>();
+ private static final List<TypeDescription> javaBeanCassandraTypes = new ArrayList<>();
+ private static volatile YamlFactory instance;
+
+ private YamlFactory()
+ {
+ loadScalarTypeDescriptions(scalarCassandraTypes);
+ loadJavaBeanTypeDescriptions(javaBeanCassandraTypes);
+ }
+
+ public static YamlFactory getInstance()
+ {
+ YamlFactory instance0 = instance;
Review Comment:
why make this mutable, its far simpler to do `private static final YamlFactory instance = new YamlFactory();`; double-check locking is an anti-pattern and far better to use class init to solve this problem
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -218,7 +222,7 @@ public static <T> T fromMap(Map<String,Object> map, boolean shouldCheck, Class<T
verifyReplacements(replacements, map);
YamlConfigurationLoader.PropertiesChecker propertiesChecker = new YamlConfigurationLoader.PropertiesChecker(replacements);
constructor.setPropertyUtils(propertiesChecker);
- Yaml yaml = new Yaml(constructor);
+ Yaml yaml = YamlFactory.getInstance().newYamlInstance(constructor, propertiesChecker);
Review Comment:
do we need `, propertiesChecker`? We did `constructor.setPropertyUtils(propertiesChecker);` already... we also override it in this method... so prob best to cleanup this relationship
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -424,5 +428,140 @@ public void check() throws ConfigurationException
logger.warn("{} parameters have been deprecated. They have new names and/or value format; For more information, please refer to NEWS.txt", deprecationWarnings);
}
}
+
+ /**
+ * Creates a YAML instance based on Cassandra's custom configuration classes and types. Yaml factory here is used
+ * to {@link org.yaml.snakeyaml.Yaml#dumpAs} and {@link org.yaml.snakeyaml.Yaml#load(String)} given object or
+ * string respectively, that in turn is used to serialize and deserialize configuration properties.
+ */
+ public static class YamlFactory
+ {
+ private static final List<TypeDescription> scalarCassandraTypes = new ArrayList<>();
+ private static final List<TypeDescription> javaBeanCassandraTypes = new ArrayList<>();
+ private static volatile YamlFactory instance;
+
+ private YamlFactory()
+ {
+ loadScalarTypeDescriptions(scalarCassandraTypes);
+ loadJavaBeanTypeDescriptions(javaBeanCassandraTypes);
+ }
+
+ public static YamlFactory getInstance()
+ {
+ YamlFactory instance0 = instance;
+ if (instance0 == null)
+ {
+ synchronized (YamlFactory.class)
+ {
+ instance0 = instance;
+ if (instance0 == null)
+ instance = instance0 = new YamlFactory();
+ }
+ }
+ return instance0;
+ }
+
+ /**
+ * Creates a new Yaml instance with the given root class.
+ * @param root The class (usually JavaBean) to be constructed.
+ * @return A new Yaml instance with the given root class.
+ */
+ public Yaml newYamlInstance(Class<?> root)
+ {
+ PropertyUtils propertyUtils = new PropertyUtils();
+ propertyUtils.setBeanAccess(BeanAccess.FIELD);
+ propertyUtils.setAllowReadOnlyProperties(true);
+ return newYamlInstance(new CustomConstructor(root, Config.class.getClassLoader()), propertyUtils);
+ }
+
+ public Yaml newYamlInstance(SafeConstructor constructor, PropertyUtils propertyUtils)
+ {
+ return create(constructor, propertyUtils);
+ }
+
+ private static Yaml create(SafeConstructor constructor, PropertyUtils propertyUtils)
+ {
+ DumperOptions options = new DumperOptions();
+ options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK);
+ options.setDefaultScalarStyle(DumperOptions.ScalarStyle.PLAIN);
+ options.setSplitLines(true);
+ options.setPrettyFlow(true); // to remove brackets around
+ Representer representer = new CassandraRepresenter(scalarCassandraTypes, options);
+ representer.setPropertyUtils(propertyUtils);
+ constructor.setPropertyUtils(propertyUtils);
+ Yaml yaml = new Yaml(constructor, representer);
+
+ scalarCassandraTypes.forEach(yaml::addTypeDescription);
+ javaBeanCassandraTypes.forEach(yaml::addTypeDescription);
+ return yaml;
+ }
+
+ private static void loadScalarTypeDescriptions(List<TypeDescription> types)
Review Comment:
rather than taking a mutable list, can this method return an immutable list? Its far better to limit mutability and have this function own that
##########
src/java/org/apache/cassandra/config/Validate.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.config;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Validate annotation to be used on configuration fields to validate the value of the field with
+ * a custom validator class and its method.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.FIELD)
+public @interface Validate
Review Comment:
This class looks like dead code? I don't see us actually use it in yaml loading/setting?
##########
src/java/org/apache/cassandra/config/WithReplacementLoader.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.config;
+
+import java.util.Map;
+
+import org.yaml.snakeyaml.introspector.Property;
+
+public class WithReplacementLoader extends DefaultLoader
+{
+ @Override
+ public Map<String, Property> getProperties(Class<?> root)
+ {
+ Map<String, Property> properties = super.getProperties(root);
+ // 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(root);
Review Comment:
You don't need a *new* method if all usage goes through this new type, you can *replace* and just drop the recursive logic
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -424,5 +428,140 @@ public void check() throws ConfigurationException
logger.warn("{} parameters have been deprecated. They have new names and/or value format; For more information, please refer to NEWS.txt", deprecationWarnings);
}
}
+
+ /**
+ * Creates a YAML instance based on Cassandra's custom configuration classes and types. Yaml factory here is used
+ * to {@link org.yaml.snakeyaml.Yaml#dumpAs} and {@link org.yaml.snakeyaml.Yaml#load(String)} given object or
+ * string respectively, that in turn is used to serialize and deserialize configuration properties.
+ */
+ public static class YamlFactory
+ {
+ private static final List<TypeDescription> scalarCassandraTypes = new ArrayList<>();
+ private static final List<TypeDescription> javaBeanCassandraTypes = new ArrayList<>();
+ private static volatile YamlFactory instance;
+
+ private YamlFactory()
+ {
+ loadScalarTypeDescriptions(scalarCassandraTypes);
+ loadJavaBeanTypeDescriptions(javaBeanCassandraTypes);
+ }
+
+ public static YamlFactory getInstance()
+ {
+ YamlFactory instance0 = instance;
+ if (instance0 == null)
+ {
+ synchronized (YamlFactory.class)
+ {
+ instance0 = instance;
+ if (instance0 == null)
+ instance = instance0 = new YamlFactory();
+ }
+ }
+ return instance0;
+ }
+
+ /**
+ * Creates a new Yaml instance with the given root class.
+ * @param root The class (usually JavaBean) to be constructed.
+ * @return A new Yaml instance with the given root class.
+ */
+ public Yaml newYamlInstance(Class<?> root)
+ {
+ PropertyUtils propertyUtils = new PropertyUtils();
+ propertyUtils.setBeanAccess(BeanAccess.FIELD);
+ propertyUtils.setAllowReadOnlyProperties(true);
+ return newYamlInstance(new CustomConstructor(root, Config.class.getClassLoader()), propertyUtils);
Review Comment:
shouldn't this be `root.getClassLoader()`?
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -424,5 +428,140 @@ public void check() throws ConfigurationException
logger.warn("{} parameters have been deprecated. They have new names and/or value format; For more information, please refer to NEWS.txt", deprecationWarnings);
}
}
+
+ /**
+ * Creates a YAML instance based on Cassandra's custom configuration classes and types. Yaml factory here is used
+ * to {@link org.yaml.snakeyaml.Yaml#dumpAs} and {@link org.yaml.snakeyaml.Yaml#load(String)} given object or
+ * string respectively, that in turn is used to serialize and deserialize configuration properties.
+ */
+ public static class YamlFactory
+ {
+ private static final List<TypeDescription> scalarCassandraTypes = new ArrayList<>();
+ private static final List<TypeDescription> javaBeanCassandraTypes = new ArrayList<>();
+ private static volatile YamlFactory instance;
+
+ private YamlFactory()
+ {
+ loadScalarTypeDescriptions(scalarCassandraTypes);
+ loadJavaBeanTypeDescriptions(javaBeanCassandraTypes);
+ }
+
+ public static YamlFactory getInstance()
+ {
+ YamlFactory instance0 = instance;
+ if (instance0 == null)
+ {
+ synchronized (YamlFactory.class)
+ {
+ instance0 = instance;
+ if (instance0 == null)
+ instance = instance0 = new YamlFactory();
+ }
+ }
+ return instance0;
+ }
+
+ /**
+ * Creates a new Yaml instance with the given root class.
+ * @param root The class (usually JavaBean) to be constructed.
+ * @return A new Yaml instance with the given root class.
+ */
+ public Yaml newYamlInstance(Class<?> root)
+ {
+ PropertyUtils propertyUtils = new PropertyUtils();
+ propertyUtils.setBeanAccess(BeanAccess.FIELD);
+ propertyUtils.setAllowReadOnlyProperties(true);
+ return newYamlInstance(new CustomConstructor(root, Config.class.getClassLoader()), propertyUtils);
+ }
+
+ public Yaml newYamlInstance(SafeConstructor constructor, PropertyUtils propertyUtils)
+ {
+ return create(constructor, propertyUtils);
+ }
+
+ private static Yaml create(SafeConstructor constructor, PropertyUtils propertyUtils)
+ {
+ DumperOptions options = new DumperOptions();
+ options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK);
+ options.setDefaultScalarStyle(DumperOptions.ScalarStyle.PLAIN);
+ options.setSplitLines(true);
+ options.setPrettyFlow(true); // to remove brackets around
+ Representer representer = new CassandraRepresenter(scalarCassandraTypes, options);
+ representer.setPropertyUtils(propertyUtils);
+ constructor.setPropertyUtils(propertyUtils);
+ Yaml yaml = new Yaml(constructor, representer);
+
+ scalarCassandraTypes.forEach(yaml::addTypeDescription);
+ javaBeanCassandraTypes.forEach(yaml::addTypeDescription);
+ return yaml;
+ }
+
+ private static void loadScalarTypeDescriptions(List<TypeDescription> types)
+ {
+ // Enum types resolved with a custom overridden handler DefaultRepresentEnum() as a smiple string.
+ types.add(createTypeDescription(DataRateSpec.LongBytesPerSecondBound.class));
+ types.add(createTypeDescription(DataStorageSpec.IntBytesBound.class));
+ types.add(createTypeDescription(DataStorageSpec.IntKibibytesBound.class));
+ types.add(createTypeDescription(DataStorageSpec.IntMebibytesBound.class));
+ types.add(createTypeDescription(DataStorageSpec.LongBytesBound.class));
+ types.add(createTypeDescription(DataStorageSpec.LongMebibytesBound.class));
+ types.add(createTypeDescription(DurationSpec.IntMillisecondsBound.class));
+ types.add(createTypeDescription(DurationSpec.IntMinutesBound.class));
+ types.add(createTypeDescription(DurationSpec.IntSecondsBound.class));
+ types.add(createTypeDescription(DurationSpec.LongMillisecondsBound.class));
+ types.add(createTypeDescription(DurationSpec.LongNanosecondsBound.class));
+ types.add(createTypeDescription(DurationSpec.LongSecondsBound.class));
+ }
+
+ private static void loadJavaBeanTypeDescriptions(List<TypeDescription> types)
Review Comment:
rather than taking a mutable list, can this method return an immutable list? Its far better to limit mutability and have this function own that
##########
src/java/org/apache/cassandra/config/Validate.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.config;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Validate annotation to be used on configuration fields to validate the value of the field with
+ * a custom validator class and its method.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.FIELD)
Review Comment:
for now this is fine, if we need to can allow in method later, just need to update the loader
##########
src/java/org/apache/cassandra/db/virtual/SettingsTable.java:
##########
@@ -59,63 +51,88 @@ final class SettingsTable extends AbstractVirtualTable
.addPartitionKeyColumn(NAME, UTF8Type.instance)
.addRegularColumn(VALUE, UTF8Type.instance)
.build());
- this.config = config;
+ }
+
+ @Override
+ protected void applyColumnDeletion(AbstractMutableVirtualTable.ColumnValues partitionKey, AbstractMutableVirtualTable.ColumnValues clusteringColumns, String columnName)
+ {
+ String name = partitionKey.value(0);
+ String key = getKeyAndWarn(name);
+ runExceptionally(() -> DatabaseDescriptor.setProperty(key, null),
+ t -> invalidRequest("Unable to clear property '%s': %s", key, t.getMessage()));
+ }
+
+ @Override
+ protected void applyColumnUpdate(AbstractMutableVirtualTable.ColumnValues partitionKey,
+ AbstractMutableVirtualTable.ColumnValues clusteringColumns,
+ Optional<AbstractMutableVirtualTable.ColumnValue> columnValue)
+ {
+ String name = partitionKey.value(0);
+ String value = columnValue.map(v -> v.value().toString()).orElse(null);
+ runExceptionally(() -> DatabaseDescriptor.setProperty(getKeyAndWarn(name), value),
+ t -> invalidRequest("Invalid update request for property '%s'. %s", name, t.getMessage()));
}
@Override
public DataSet data(DecoratedKey partitionKey)
{
SimpleDataSet result = new SimpleDataSet(metadata());
String name = UTF8Type.instance.compose(partitionKey.getKey());
- if (BACKWARDS_COMPATABLE_NAMES.containsKey(name))
- ClientWarn.instance.warn("key '" + name + "' is deprecated; should switch to '" + BACKWARDS_COMPATABLE_NAMES.get(name) + "'");
- if (PROPERTIES.containsKey(name))
- result.row(name).column(VALUE, getValue(PROPERTIES.get(name)));
+ try
+ {
+ Object value = getStringProperty(getKeyAndWarn(name));
+ result.row(name).column(VALUE, value);
+ }
+ catch (PropertyNotFoundException e)
+ {
+ // ignore
+ }
return result;
}
@Override
public DataSet data()
{
SimpleDataSet result = new SimpleDataSet(metadata());
- for (Map.Entry<String, Property> e : PROPERTIES.entrySet())
- result.row(e.getKey()).column(VALUE, getValue(e.getValue()));
+ DatabaseDescriptor.visit(withBackwardsCompatableNamesVisitor((key, type, ro) ->
+ result.row(key).column(VALUE, getStringProperty(BACKWARDS_COMPATABLE_NAMES.getOrDefault(key, key)))),
+ t -> new ConfigurationException(t.getMessage(), false));
return result;
}
- private String getValue(Property prop)
+ static String getKeyAndWarn(String name)
{
- Object value = prop.get(config);
- return value == null ? null : value.toString();
+ String key = BACKWARDS_COMPATABLE_NAMES.getOrDefault(name, name);
+ if (BACKWARDS_COMPATABLE_NAMES.containsKey(name))
+ ClientWarn.instance.warn("key '" + name + "' is deprecated; should switch to '" + BACKWARDS_COMPATABLE_NAMES.get(name) + '\'');
+ return key;
+ }
+
+ static DatabaseDescriptor.PropertyVisitor withBackwardsCompatableNamesVisitor(DatabaseDescriptor.PropertyVisitor visitor)
+ {
+ BiMap<String, String> inversedNames = BACKWARDS_COMPATABLE_NAMES.inverse();
+ return (name, type, readOnly) -> {
+ if (inversedNames.containsKey(name))
+ visitor.visit(inversedNames.get(name), type, readOnly);
+ visitor.visit(name, type, readOnly);
+ };
Review Comment:
Can you remove this concept, this adds more complexity than we need... We need a few things
# On Init
8 did someone add a new config that conflicts with one of the old names defined by this class? If so fail fast (needed so that config doesn't get added and is blocked before merge)
# At runtime
* any usage of the old name gets mapped to the new name
given this, `getKeyAndWarn(name)` is all we need, and a `visitor` is too heavy weight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org