From 44ace88585f2aefc322465ed6b6f6ad5bfd5bb34 Mon Sep 17 00:00:00 2001 From: Ekaterina Dimitrova Date: Mon, 15 Nov 2021 17:30:45 -0500 Subject: [PATCH] internode_send_buff_size_in_bytes and internode_recv_buff_size_in_bytes have new names. Backward compatibility with the old names added patch by Ekaterina Dimitrova; reviewed by David Capwell for CASSANDRA-17141 --- .build/build-rat.xml | 1 + CHANGES.txt | 1 + NEWS.txt | 7 +- conf/cassandra.yaml | 4 +- .../org/apache/cassandra/config/Config.java | 2 + .../org/apache/cassandra/config/Replaces.java | 45 +++++ .../apache/cassandra/config/ReplacesList.java | 34 ++++ .../config/YamlConfigurationLoader.java | 163 +++++++++++++++++- ...cassandra_deprecated_parameters_names.yaml | 56 ++++++ .../LoadOldYAMLBackwardCompatibilityTest.java | 44 +++++ .../config/YamlConfigurationLoaderTest.java | 14 +- 11 files changed, 359 insertions(+), 12 deletions(-) create mode 100644 src/java/org/apache/cassandra/config/Replaces.java create mode 100644 src/java/org/apache/cassandra/config/ReplacesList.java create mode 100644 test/conf/cassandra_deprecated_parameters_names.yaml create mode 100644 test/unit/org/apache/cassandra/config/LoadOldYAMLBackwardCompatibilityTest.java diff --git a/.build/build-rat.xml b/.build/build-rat.xml index ce4e6f4afb..47e2cdfa9c 100644 --- a/.build/build-rat.xml +++ b/.build/build-rat.xml @@ -54,6 +54,7 @@ + diff --git a/CHANGES.txt b/CHANGES.txt index 1a58cdf1ad..cb8a8a9637 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.0.2 + * internode_send_buff_size_in_bytes and internode_recv_buff_size_in_bytes have new names. Backward compatibility with the old names added (CASSANDRA-17141) * Remove unused configuration parameters from cassandra.yaml (CASSANDRA-17132) * Queries performed with NODE_LOCAL consistency level do not update request metrics (CASSANDRA-17052) * Fix multiple full sources can be select unexpectedly for bootstrap streaming (CASSANDRA-16945) diff --git a/NEWS.txt b/NEWS.txt index db846c8d29..5460aa2a7b 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -43,7 +43,12 @@ Upgrading confirm it is set to value lower than 31 otherwise Cassandra will fail to start. See CASSANDRA-9384 for further details. You also need to regenerate passwords for users for who the password was created while the above property was set to be more than 30 otherwise they will not be able to log in. - + - As part of the Internode Messaging improvement work in CASSANDRA-15066, internode_send_buff_size_in_bytes and + internode_recv_buff_size_in_bytes were renamed to internode_socket_send_buffer_size_in_bytes and + internode_socket_receive_buffer_size_in_bytes. To support upgrades pre-4.0, we add backward compatibility and + currently both old and new names should work. Cassandra 4.0.0 and Cassandra 4.0.1 work ONLY with the new names + (They weren't updated in cassandra.yaml though). + 4.0 === diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index ff30792c00..451f9df4c8 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -773,12 +773,12 @@ rpc_keepalive: true # /proc/sys/net/ipv4/tcp_wmem # /proc/sys/net/ipv4/tcp_wmem # and 'man tcp' -# internode_send_buff_size_in_bytes: +# internode_socket_send_buffer_size_in_bytes: # Uncomment to set socket buffer size for internode communication # Note that when setting this, the buffer size is limited by net.core.wmem_max # and when not setting it it is defined by net.ipv4.tcp_wmem -# internode_recv_buff_size_in_bytes: +# internode_socket_receive_buffer_size_in_bytes: # Set to true to have Cassandra create a hard link to each sstable # flushed or streamed locally in a backups/ subdirectory of the diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index 2bf4e769d6..2e1d973e29 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -158,7 +158,9 @@ public class Config public Integer internode_max_message_size_in_bytes; + @Replaces(oldName = "internode_send_buff_size_in_bytes", deprecated = true) public int internode_socket_send_buffer_size_in_bytes = 0; + @Replaces(oldName = "internode_recv_buff_size_in_bytes", deprecated = true) public int internode_socket_receive_buffer_size_in_bytes = 0; // TODO: derive defaults from system memory settings? diff --git a/src/java/org/apache/cassandra/config/Replaces.java b/src/java/org/apache/cassandra/config/Replaces.java new file mode 100644 index 0000000000..93bdcb5fb3 --- /dev/null +++ b/src/java/org/apache/cassandra/config/Replaces.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.lang.annotation.ElementType; +import java.lang.annotation.Repeatable; +import java.lang.annotation.Retention; +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) + */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ ElementType.FIELD}) +@Repeatable(ReplacesList.class) +public @interface Replaces +{ + /** + * @return old configuration parameter name + */ + String oldName(); + + /** + * @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 new file mode 100644 index 0000000000..a2b0c967c0 --- /dev/null +++ b/src/java/org/apache/cassandra/config/ReplacesList.java @@ -0,0 +1,34 @@ +/* + * 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; + +/** + * Contatining annotation type for the repeatable annotation {@link Replaces} + */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ ElementType.FIELD}) +public @interface ReplacesList +{ + Replaces[] value(); +} diff --git a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java index a774a8154e..05b18bebb9 100644 --- a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java +++ b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java @@ -22,14 +22,18 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; import java.lang.annotation.Annotation; +import java.lang.reflect.Field; import java.net.URL; +import java.util.ArrayList; import java.util.Collections; +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 com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -123,7 +127,8 @@ public class YamlConfigurationLoader implements ConfigurationLoader Constructor constructor = new CustomConstructor(Config.class, Yaml.class.getClassLoader()); - PropertiesChecker propertiesChecker = new PropertiesChecker(); + Map, Map> replacements = getNameReplacements(Config.class); + PropertiesChecker propertiesChecker = new PropertiesChecker(replacements); constructor.setPropertyUtils(propertiesChecker); Yaml yaml = new Yaml(constructor); Config result = loadConfig(yaml, configBytes); @@ -137,6 +142,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader } } + @VisibleForTesting public static T fromMap(Map map, Class klass) { return fromMap(map, true, klass); @@ -146,7 +152,8 @@ public class YamlConfigurationLoader implements ConfigurationLoader public static T fromMap(Map map, boolean shouldCheck, Class klass) { Constructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader()); - YamlConfigurationLoader.PropertiesChecker propertiesChecker = new YamlConfigurationLoader.PropertiesChecker(); + Map, Map> replacements = getNameReplacements(Config.class); + YamlConfigurationLoader.PropertiesChecker propertiesChecker = new YamlConfigurationLoader.PropertiesChecker(replacements); constructor.setPropertyUtils(propertiesChecker); Yaml yaml = new Yaml(constructor); Node node = yaml.represent(map); @@ -212,15 +219,66 @@ public class YamlConfigurationLoader implements ConfigurationLoader private final Set nullProperties = new HashSet<>(); - public PropertiesChecker() + private final Map, Map> replacements; + + public PropertiesChecker(Map, Map> replacements) { + this.replacements = Objects.requireNonNull(replacements, "Replacements should not be null"); setSkipMissingProperties(true); } @Override public Property getProperty(Class type, String name) { - final Property result = super.getProperty(type, name); + final Property result; + Map 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()) + { + @Override + public Class[] getActualTypeArguments() + { + return newProperty.getActualTypeArguments(); + } + + @Override + public void set(Object o, Object o1) throws Exception + { + newProperty.set(o, o1); + } + + @Override + public Object get(Object o) + { + return newProperty.get(o); + } + + @Override + public List getAnnotations() + { + return null; + } + + @Override + public A getAnnotation(Class aClass) + { + return null; + } + }; + + if(replacement.deprecated) + { + logger.warn("{} parameter has been deprecated. It has a new name; For more information, please refer to NEWS.txt", name); + } + } + else + { + result = super.getProperty(type, name); + } + if (result instanceof MissingProperty) { @@ -251,11 +309,13 @@ public class YamlConfigurationLoader implements ConfigurationLoader return result.get(object); } + @Override public List getAnnotations() { return Collections.EMPTY_LIST; } + @Override public A getAnnotation(Class aClass) { return null; @@ -276,4 +336,97 @@ public class YamlConfigurationLoader implements ConfigurationLoader } } } + + /** + * @param klass to get replacements for + * @return map of old names and replacements needed. + */ + private static Map, Map> getNameReplacements(Class klass) + { + List replacements = getReplacements(klass); + Map, Map> objectOldNames = new HashMap<>(); + for (Replacement r : replacements) + { + Map oldNames = objectOldNames.computeIfAbsent(r.parent, ignore -> new HashMap<>()); + if (!oldNames.containsKey(r.oldName)) + oldNames.put(r.oldName, r); + else + { + throw new ConfigurationException("Invalid annotations, you have more than one @Replaces annotation in " + + "Config class with same old name(" + r.oldName + ") defined."); + } + } + return objectOldNames; + } + + private static List getReplacements(Class klass) + { + List replacements = new ArrayList<>(); + for (Field field : klass.getDeclaredFields()) + { + String newName = field.getName(); + 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); + } + else + { + for (ReplacesList replacesList : byType) + for (Replaces r : replacesList.value()) + addReplacement(klass, replacements, newName, r); + } + } + return replacements.isEmpty() ? Collections.emptyList() : replacements; + } + + private static void addReplacement(Class klass, + List replacements, + String newName, + Replaces r) + { + String oldName = r.oldName(); + boolean deprecated = r.deprecated(); + + replacements.add(new Replacement(klass, oldName, newName, 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) + */ + static final class Replacement + { + /** + * Currently we use for Config class + */ + final Class parent; + /** + * Old name of the configuration parameter + */ + final String oldName; + /** + * 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. + */ + final boolean deprecated; + + Replacement(Class parent, + String oldName, + String newName, + boolean deprecated) + { + this.parent = Objects.requireNonNull(parent); + this.oldName = Objects.requireNonNull(oldName); + this.newName = Objects.requireNonNull(newName); + // by default deprecated is false + this.deprecated = deprecated; + } + } } diff --git a/test/conf/cassandra_deprecated_parameters_names.yaml b/test/conf/cassandra_deprecated_parameters_names.yaml new file mode 100644 index 0000000000..3258b7faeb --- /dev/null +++ b/test/conf/cassandra_deprecated_parameters_names.yaml @@ -0,0 +1,56 @@ +# +# Warning! +# Consider the effects on 'o.a.c.i.s.LegacySSTableTest' before changing schemas in this file. +# +cluster_name: Test Cluster +# memtable_allocation_type: heap_buffers +memtable_allocation_type: offheap_objects +commitlog_sync: batch +commitlog_sync_batch_window_in_ms: 1.0 +commitlog_segment_size_in_mb: 5 +commitlog_directory: build/test/cassandra/commitlog +# commitlog_compression: +# - class_name: LZ4Compressor +cdc_raw_directory: build/test/cassandra/cdc_raw +cdc_enabled: false +hints_directory: build/test/cassandra/hints +partitioner: org.apache.cassandra.dht.ByteOrderedPartitioner +listen_address: 127.0.0.1 +storage_port: 7012 +ssl_storage_port: 17012 +start_native_transport: true +native_transport_port: 9042 +column_index_size_in_kb: 4 +saved_caches_directory: build/test/cassandra/saved_caches +data_file_directories: + - build/test/cassandra/data +disk_access_mode: mmap +seed_provider: + - class_name: org.apache.cassandra.locator.SimpleSeedProvider + parameters: + - seeds: "127.0.0.1:7012" +endpoint_snitch: org.apache.cassandra.locator.SimpleSnitch +dynamic_snitch: true +server_encryption_options: + internode_encryption: none + keystore: conf/.keystore + keystore_password: cassandra + truststore: conf/.truststore + truststore_password: cassandra +incremental_backups: true +concurrent_compactors: 4 +compaction_throughput_mb_per_sec: 0 +row_cache_class_name: org.apache.cassandra.cache.OHCProvider +row_cache_size_in_mb: 16 +enable_user_defined_functions: true +enable_scripted_user_defined_functions: true +prepared_statements_cache_size_mb: 1 +corrupted_tombstone_strategy: exception +stream_entire_sstables: true +stream_throughput_outbound_megabits_per_sec: 200000000 +enable_sasi_indexes: true +enable_materialized_views: true +enable_drop_compact_storage: true +file_cache_enabled: true +internode_send_buff_size_in_bytes: 5 +internode_recv_buff_size_in_bytes: 5 diff --git a/test/unit/org/apache/cassandra/config/LoadOldYAMLBackwardCompatibilityTest.java b/test/unit/org/apache/cassandra/config/LoadOldYAMLBackwardCompatibilityTest.java new file mode 100644 index 0000000000..575f04d432 --- /dev/null +++ b/test/unit/org/apache/cassandra/config/LoadOldYAMLBackwardCompatibilityTest.java @@ -0,0 +1,44 @@ +/* + * 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 org.junit.BeforeClass; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class LoadOldYAMLBackwardCompatibilityTest +{ + @BeforeClass + public static void setupDatabaseDescriptor() + { + System.setProperty("cassandra.config", "cassandra_deprecated_parameters_names.yaml"); + DatabaseDescriptor.daemonInitialization(); + } + + // CASSANDRA-17141 + @Test + public void testConfigurationLoaderBackwardCompatibility() + { + Config config = DatabaseDescriptor.loadConfig(); + //Confirm parameters were successfully read with the old names from cassandra-old.yaml + assertEquals(5, config.internode_socket_send_buffer_size_in_bytes); + assertEquals(5, config.internode_socket_receive_buffer_size_in_bytes); + } +} diff --git a/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java b/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java index 2aff83f5b9..3b7e64edda 100644 --- a/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java +++ b/test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java @@ -38,10 +38,14 @@ public class YamlConfigurationLoaderTest Map encryptionOptions = ImmutableMap.of("cipher_suites", Collections.singletonList("FakeCipher"), "optional", false, "enabled", true); - Map map = ImmutableMap.of("storage_port", storagePort, - "commitlog_sync", commitLogSync, - "seed_provider", seedProvider, - "client_encryption_options", encryptionOptions); + Map map = new ImmutableMap.Builder() + .put("storage_port", storagePort) + .put("commitlog_sync", commitLogSync) + .put("seed_provider", seedProvider) + .put("client_encryption_options", encryptionOptions) + .put("internode_send_buff_size_in_bytes", 5) + .put("internode_recv_buff_size_in_bytes", 5) + .build(); Config config = YamlConfigurationLoader.fromMap(map, Config.class); assertEquals(storagePort, config.storage_port); // Check a simple integer @@ -49,5 +53,7 @@ public class YamlConfigurationLoaderTest assertEquals(seedProvider, config.seed_provider); // Check a parameterized class assertEquals(false, config.client_encryption_options.optional); // Check a nested object assertEquals(true, config.client_encryption_options.enabled); // Check a nested object + assertEquals(5, config.internode_socket_send_buffer_size_in_bytes); // Check names backward compatibility (CASSANDRA-17141) + assertEquals(5, config.internode_socket_receive_buffer_size_in_bytes); // Check names backward compatibility (CASSANDRA-17141) } }