You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by ed...@apache.org on 2022/02/06 03:44:15 UTC
[cassandra] 02/13: Backward compatibility framework for configuration parameters patch by Ekaterina Dimitrova; reviewed by Caleb Rackliffe, David Capwell, Michael Semb Wever and Benjamin Lerer for CASSANDRA-15234
This is an automated email from the ASF dual-hosted git repository.
edimitrova pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/cassandra.git
commit 9c6b382058578ac75b88055a13aa83944901fb88
Author: Ekaterina Dimitrova <ek...@datastax.com>
AuthorDate: Tue Dec 14 23:04:43 2021 -0500
Backward compatibility framework for configuration parameters
patch by Ekaterina Dimitrova; reviewed by Caleb Rackliffe, David Capwell, Michael Semb Wever and Benjamin Lerer for CASSANDRA-15234
---
.../org/apache/cassandra/config/Converters.java | 125 +++++++++++++++++++
src/java/org/apache/cassandra/config/Replaces.java | 9 +-
.../org/apache/cassandra/config/ReplacesList.java | 2 +-
.../cassandra/config/YamlConfigurationLoader.java | 134 ++++++++++++++++++---
.../apache/cassandra/db/virtual/SettingsTable.java | 16 +++
5 files changed, 263 insertions(+), 23 deletions(-)
diff --git a/src/java/org/apache/cassandra/config/Converters.java b/src/java/org/apache/cassandra/config/Converters.java
new file mode 100644
index 0000000..9a832cc
--- /dev/null
+++ b/src/java/org/apache/cassandra/config/Converters.java
@@ -0,0 +1,125 @@
+/*
+ * 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.function.Function;
+
+/**
+ * Converters for backward compatibility with the old cassandra.yaml where duration, data rate and
+ * data storage configuration parameters were provided only by value and the expected unit was part of the configuration
+ * parameter name(suffix). (CASSANDRA-15234)
+ * It is important to be noted that this converter is not intended to be used when we don't change name of a configuration
+ * parameter but we want to add unit. This would always default to the old value provided without a unit at the moment.
+ * In case this functionality is needed at some point, please, raise a Jira ticket.
+ */
+public enum Converters
+{
+ /**
+ * This converter is used when we change the name of a cassandra.yaml configuration parameter but we want to be
+ * able to still use the old name too. No units involved.
+ */
+ IDENTITY(null, o -> o, o-> o),
+ MILLIS_DURATION(Long.class,
+ o -> DurationSpec.inMilliseconds((Long) o),
+ o -> ((DurationSpec)o).toMilliseconds()),
+ MILLIS_DOUBLE_DURATION(Double.class,
+ o -> DurationSpec.inDoubleMilliseconds((Double) o),
+ o -> ((DurationSpec)o).toMilliseconds()),
+ /**
+ * This converter is used to support backward compatibility for parameters where in the past -1 was used as a value
+ * Example: credentials_update_interval_in_ms = -1 and credentials_update_interval = null (quantity of 0ms) are equal.
+ */
+ MILLIS_CUSTOM_DURATION(Long.class,
+ o -> (Long)o == -1 ? new DurationSpec("0ms") : DurationSpec.inMilliseconds((Long) o),
+ o -> ((DurationSpec)o).toMilliseconds() == 0 ? -1 : ((DurationSpec)o).toMilliseconds()),
+ SECONDS_DURATION(Long.class,
+ o -> DurationSpec.inSeconds((Long) o),
+ o -> ((DurationSpec)o).toSeconds()),
+ MINUTES_DURATION(Long.class,
+ o -> DurationSpec.inMinutes((Long) o),
+ o -> ((DurationSpec)o).toMinutes()),
+ MEBIBYTES_DATA_STORAGE(Long.class,
+ o -> DataStorageSpec.inMebibytes((Long) o),
+ o -> ((DataStorageSpec)o).toMebibytes()),
+ KIBIBYTES_DATASTORAGE(Long.class,
+ o -> DataStorageSpec.inKibibytes((Long) o),
+ o -> ((DataStorageSpec)o).toKibibytes()),
+ BYTES_DATASTORAGE(Long.class,
+ o -> DataStorageSpec.inBytes((Long) o),
+ o -> ((DataStorageSpec)o).toBytes()),
+ MEBIBYTES_PER_SECOND_DATA_RATE(Long.class,
+ o -> DataRateSpec.inMebibytesPerSecond((Long) o),
+ o -> ((DataRateSpec)o).toMebibytesPerSecond()),
+ /**
+ * This converter is a custom one to support backward compatibility for stream_throughput_outbound and
+ * inter_dc_stream_throughput_outbound which were provided in megatibs per second prior CASSANDRA-15234.
+ */
+ MEGABITS_TO_MEBIBYTES_PER_SECOND_DATA_RATE(Long.class,
+ o -> DataRateSpec.megabitsPerSecondInMebibytesPerSecond((Long)o),
+ o -> ((DataRateSpec)o).toMegabitsPerSecond());
+
+ private final Class<?> inputType;
+ private final Function<Object, Object> convert;
+ private final Function<Object, Object> reverseConvert;
+
+ Converters(Class<?> inputType, Function<Object, Object> convert, Function<Object, Object> reverseConvert)
+ {
+ this.inputType = inputType;
+ this.convert = convert;
+ this.reverseConvert = reverseConvert;
+ }
+
+ /**
+ * A method to identify what type is needed to be returned by the converter used for a configuration parameter
+ * in {@link Replaces} annotation in {@link Config}
+ *
+ * @return class type
+ */
+ public Class<?> getInputType()
+ {
+ return inputType;
+ }
+
+ /**
+ * Apply the converter specified as part of the {@link Replaces} annotation in {@link Config}
+ *
+ * @param value we will use from cassandra.yaml to create a new {@link Config} parameter of type {@link DurationSpec},
+ * {@link DataRateSpec} or {@link DataStorageSpec}
+ * @return new object of type {@link DurationSpec}, {@link DataRateSpec} or {@link DataStorageSpec}
+ */
+ public Object convert(Object value)
+ {
+ if (value == null) return null;
+ return convert.apply(value);
+ }
+
+ /**
+ * Apply the converter specified as part of the {@link Replaces} annotation in {@link Config} to get config parameters'
+ * values in the old format pre-CASSANDRA-15234 and in the right units, used in the Virtual Tables to ensure backward
+ * compatibility
+ *
+ * @param value we will use to calculate the output value
+ * @return the numeric value
+ */
+ public Object deconvert(Object value)
+ {
+ if (value == null) return 0;
+ return reverseConvert.apply(value);
+ }
+}
diff --git a/src/java/org/apache/cassandra/config/Replaces.java b/src/java/org/apache/cassandra/config/Replaces.java
index 93bdcb5..e1354b9 100644
--- a/src/java/org/apache/cassandra/config/Replaces.java
+++ b/src/java/org/apache/cassandra/config/Replaces.java
@@ -25,8 +25,8 @@ import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
/**
- * Repeatable annotation for providing old name and whether the
- * config parameters we annotate are deprecated and we need to warn the users. (CASSANDRA-17141)
+ * Repeatable annotation for providing old name, converter from old to new type and whether the
+ * config parameters we annotate are deprecated and we need to warn the users. (CASSANDRA-17141, CASSANDRA-15234)
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.FIELD})
@@ -39,6 +39,11 @@ public @interface Replaces
String oldName();
/**
+ * @return which converter we need depending on the old default unit that was used
+ */
+ Converters converter() default Converters.IDENTITY;
+
+ /**
* @return whether the parameter should be marked as deprecated or not and warning sent to the user
*/
boolean deprecated() default false;
diff --git a/src/java/org/apache/cassandra/config/ReplacesList.java b/src/java/org/apache/cassandra/config/ReplacesList.java
index a2b0c96..eec4e4c 100644
--- a/src/java/org/apache/cassandra/config/ReplacesList.java
+++ b/src/java/org/apache/cassandra/config/ReplacesList.java
@@ -24,7 +24,7 @@ import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
/**
- * Contatining annotation type for the repeatable annotation {@link Replaces}
+ * Concatenating annotation type for the repeatable annotation {@link Replaces}
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.FIELD})
diff --git a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
index dc9b7fb..4fc82b4 100644
--- a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
+++ b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java
@@ -25,12 +25,16 @@ import java.lang.reflect.Field;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
+import java.nio.file.Files;
+import java.nio.file.Paths;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
@@ -38,9 +42,10 @@ import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.io.ByteStreams;
-import org.apache.cassandra.io.util.File;
import org.apache.commons.lang3.SystemUtils;
+import org.apache.cassandra.io.util.File;
+
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -65,6 +70,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
/**
* Inspect the classpath to find storage configuration file
*/
+ @VisibleForTesting
private static URL getStorageConfigURL() throws ConfigurationException
{
String configUrl = System.getProperty("cassandra.config");
@@ -106,6 +112,8 @@ public class YamlConfigurationLoader implements ConfigurationLoader
{
if (storageConfigURL == null)
storageConfigURL = getStorageConfigURL();
+
+ validateConfigFile();
return loadConfig(storageConfigURL);
}
@@ -125,7 +133,6 @@ public class YamlConfigurationLoader implements ConfigurationLoader
throw new AssertionError(e);
}
-
Constructor constructor = new CustomConstructor(Config.class, Yaml.class.getClassLoader());
Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
PropertiesChecker propertiesChecker = new PropertiesChecker(replacements);
@@ -138,8 +145,48 @@ public class YamlConfigurationLoader implements ConfigurationLoader
catch (YAMLException e)
{
throw new ConfigurationException("Invalid yaml: " + url + SystemUtils.LINE_SEPARATOR
- + " Error: " + e.getMessage(), false);
+ + " Error: " + e.getMessage(), false);
+ }
+ }
+
+ private static String readStorageConfig(URL url)
+ {
+ String content;
+
+ try
+ {
+ content = new String (Files.readAllBytes(Paths.get(String.valueOf(url).substring(5))));
}
+ catch (IOException e)
+ {
+ throw new ConfigurationException("Invalid yaml: " + url, e);
+ }
+
+ return content;
+ }
+
+ private static void validateConfigFile()
+ {
+ String content = YamlConfigurationLoader.readStorageConfig(storageConfigURL);
+
+ if (isBlank("commitlog_sync_period", content))
+ throw new IllegalArgumentException("You should provide a value for commitlog_sync_period or comment it in " +
+ "order to get a default one");
+
+ if (isBlank("commitlog_sync_group_window", content))
+ throw new IllegalArgumentException("You should provide a value for commitlog_sync_group_window or comment it in " +
+ "order to get a default one");
+ }
+
+ /**
+ * This method helps to preserve the behavior of parameters which were originally of primitive type and
+ * without default value in Config.java (CASSANDRA-15234)
+ */
+ private static boolean isBlank(String property, String content)
+ {
+ Pattern p = Pattern.compile(String.format("%s%s *: *$", '^', property), Pattern.MULTILINE);
+ Matcher m = p.matcher(content);
+ return m.find();
}
@VisibleForTesting
@@ -171,6 +218,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
return value;
}
+ @VisibleForTesting
static class CustomConstructor extends CustomClassLoaderConstructor
{
CustomConstructor(Class<?> theRoot, ClassLoader classLoader)
@@ -213,6 +261,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
* Utility class to check that there are no extra properties and that properties that are not null by default
* are not set to null.
*/
+ @VisibleForTesting
private static class PropertiesChecker extends PropertyUtils
{
private final Set<String> missingProperties = new HashSet<>();
@@ -223,22 +272,23 @@ public class YamlConfigurationLoader implements ConfigurationLoader
private final Map<Class<?>, Map<String, Replacement>> replacements;
- public PropertiesChecker(Map<Class<?>, Map<String, Replacement>> replacements)
+ PropertiesChecker(Map<Class<?>, Map<String, Replacement>> replacements)
{
this.replacements = Objects.requireNonNull(replacements, "Replacements should not be null");
setSkipMissingProperties(true);
}
@Override
- public Property getProperty(Class<? extends Object> type, String name)
+ public Property getProperty(Class<?> type, String name)
{
final Property result;
Map<String, Replacement> typeReplacements = replacements.getOrDefault(type, Collections.emptyMap());
if (typeReplacements.containsKey(name))
{
Replacement replacement = typeReplacements.get(name);
+
final Property newProperty = super.getProperty(type, replacement.newName);
- result = new Property(replacement.oldName, newProperty.getType())
+ result = new Property(replacement.oldName, replacement.oldType)
{
@Override
public Class<?>[] getActualTypeArguments()
@@ -249,7 +299,8 @@ public class YamlConfigurationLoader implements ConfigurationLoader
@Override
public void set(Object o, Object o1) throws Exception
{
- newProperty.set(o, o1);
+ Object migratedValue = replacement.converter.convert(o1);
+ newProperty.set(o, migratedValue);
}
@Override
@@ -270,7 +321,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
return null;
}
};
-
+
if (replacement.deprecated)
deprecationWarnings.add(replacement.oldName);
}
@@ -293,6 +344,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
{
nullProperties.add(getName());
}
+
result.set(object, value);
}
@@ -331,7 +383,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
throw new ConfigurationException("Invalid yaml. Please remove properties " + missingProperties + " from your cassandra.yaml", false);
if (!deprecationWarnings.isEmpty())
- logger.warn("{} parameters have been deprecated. They have new names; For more information, please refer to NEWS.txt", deprecationWarnings);
+ logger.warn("{} parameters have been deprecated. They have new names and/or value format; For more information, please refer to NEWS.txt", deprecationWarnings);
}
}
@@ -339,9 +391,9 @@ public class YamlConfigurationLoader implements ConfigurationLoader
* @param klass to get replacements for
* @return map of old names and replacements needed.
*/
- private static Map<Class<?>, Map<String, Replacement>> getNameReplacements(Class<?> klass)
+ private static Map<Class<? extends Object>, Map<String, Replacement>> getNameReplacements(Class<? extends Object> klass)
{
- List<Replacement> replacements = getReplacements(klass);
+ List<Replacement> replacements = getReplacementsRecursive(klass);
Map<Class<?>, Map<String, Replacement>> objectOldNames = new HashMap<>();
for (Replacement r : replacements)
{
@@ -357,24 +409,52 @@ public class YamlConfigurationLoader implements ConfigurationLoader
return objectOldNames;
}
+ /**
+ * @param klass to get replacements for
+ * @return map of old names and replacements needed.
+ */
+ private static List<Replacement> getReplacementsRecursive(Class<?> klass)
+ {
+ Set<Class<?>> seen = new HashSet<>(); // to make sure not to process the same type twice
+ List<Replacement> accum = new ArrayList<>();
+ getReplacementsRecursive(seen, accum, klass);
+ return accum.isEmpty() ? Collections.emptyList() : accum;
+ }
+
+ private static void getReplacementsRecursive(Set<Class<?>> seen,
+ List<Replacement> accum,
+ Class<?> klass)
+ {
+ accum.addAll(getReplacements(klass));
+ for (Field field : klass.getDeclaredFields())
+ {
+ if (seen.add(field.getType()))
+ {
+ // first time looking at this type, walk it
+ getReplacementsRecursive(seen, accum, field.getType());
+ }
+ }
+ }
+
private static List<Replacement> getReplacements(Class<?> klass)
{
List<Replacement> replacements = new ArrayList<>();
for (Field field : klass.getDeclaredFields())
{
String newName = field.getName();
+ Class<?> newType = field.getType();
final ReplacesList[] byType = field.getAnnotationsByType(ReplacesList.class);
if (byType == null || byType.length == 0)
{
Replaces r = field.getAnnotation(Replaces.class);
if (r != null)
- addReplacement(klass, replacements, newName, r);
+ addReplacement(klass, replacements, newName, newType, r);
}
else
{
for (ReplacesList replacesList : byType)
for (Replaces r : replacesList.value())
- addReplacement(klass, replacements, newName, r);
+ addReplacement(klass, replacements, newName, newType, r);
}
}
return replacements.isEmpty() ? Collections.emptyList() : replacements;
@@ -382,24 +462,28 @@ public class YamlConfigurationLoader implements ConfigurationLoader
private static void addReplacement(Class<?> klass,
List<Replacement> replacements,
- String newName,
+ String newName, Class<?> newType,
Replaces r)
{
String oldName = r.oldName();
+
boolean deprecated = r.deprecated();
- replacements.add(new Replacement(klass, oldName, newName, deprecated));
+ Class<?> oldType = r.converter().getInputType();
+ if (oldType == null)
+ oldType = newType;
+
+ replacements.add(new Replacement(klass, oldName, oldType, newName, r.converter(), deprecated));
}
/**
- * Holder for replacements to support backward compatibility between old and new names for configuration parameters
- * backported partially from trunk(CASSANDRA-15234) to support a bug fix/improvement in Cassadra 4.0
- * (CASSANDRA-17141)
+ * Holder for replacements to support backward compatibility between old and new names and types
+ * of configuration parameters (CASSANDRA-15234)
*/
static final class Replacement
{
/**
- * Currently we use for Config class
+ * Currently we use Config class
*/
final Class<?> parent;
/**
@@ -407,22 +491,32 @@ public class YamlConfigurationLoader implements ConfigurationLoader
*/
final String oldName;
/**
+ * Old type of the configuration parameter
+ */
+ final Class<?> oldType;
+ /**
* New name used for the configuration parameter
*/
final String newName;
/**
- * A flag to mark whether the old name is deprecated and fire a warning to the user. By default we set it to false.
+ * Converter to be used according to the old default unit which was provided as a suffix of the configuration
+ * parameter
*/
+ final Converters converter;
final boolean deprecated;
Replacement(Class<?> parent,
String oldName,
+ Class<?> oldType,
String newName,
+ Converters converter,
boolean deprecated)
{
this.parent = Objects.requireNonNull(parent);
this.oldName = Objects.requireNonNull(oldName);
+ this.oldType = Objects.requireNonNull(oldType);
this.newName = Objects.requireNonNull(newName);
+ this.converter = Objects.requireNonNull(converter);
// by default deprecated is false
this.deprecated = deprecated;
}
diff --git a/src/java/org/apache/cassandra/db/virtual/SettingsTable.java b/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
index b0ae018..cc91152 100644
--- a/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
+++ b/src/java/org/apache/cassandra/db/virtual/SettingsTable.java
@@ -50,6 +50,13 @@ final class SettingsTable extends AbstractVirtualTable
.collect(Collectors.toMap(Field::getName, Functions.identity()));
@VisibleForTesting
+ static final Map<String, Field> ANNOTATED_FIELDS =
+ Arrays.stream(Config.class.getFields())
+ .filter(f -> !Modifier.isStatic(f.getModifiers()))
+ .filter(f -> f.isAnnotationPresent(Replaces.class))
+ .collect(Collectors.toMap(Field::getName, Functions.identity()));
+
+ @VisibleForTesting
final Map<String, BiConsumer<SimpleDataSet, Field>> overrides =
ImmutableMap.<String, BiConsumer<SimpleDataSet, Field>>builder()
.put("audit_logging_options", this::addAuditLoggingOptions)
@@ -108,6 +115,15 @@ final class SettingsTable extends AbstractVirtualTable
if (value.getClass().isArray())
value = Arrays.toString((Object[]) value);
result.row(f.getName()).column(VALUE, value.toString());
+
+ if (ANNOTATED_FIELDS.containsKey(f.getName()))
+ {
+ Replaces annotation = f.getAnnotation(Replaces.class);
+ result.row(annotation.oldName())
+ .column(VALUE, annotation.converter()
+ .deconvert(value)
+ .toString());
+ }
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org