[GitHub] [cassandra] dcapwell commented on a diff in pull request #1557: CASSANDRA-17379: don't start if old and new config params have different values

[GitBox <gi...@apache.org>]

dcapwell commented on code in PR #1557:
URL: https://github.com/apache/cassandra/pull/1557#discussion_r861028820


##########
.build/build-rat.xml:
##########
@@ -58,6 +58,7 @@
                  <exclude NAME="**/doc/antora.yml"/>
                  <exclude name="**/test/conf/cassandra.yaml"/>
                  <exclude name="**/test/conf/cassandra-old.yaml"/>
+                 <exclude name="**/test/conf/cassandra-dupes-template.yaml"/>

Review Comment:
   I don't see this file in this patch



##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -71,7 +73,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
      * Inspect the classpath to find storage configuration file
      */
     @VisibleForTesting
-    private static URL getStorageConfigURL() throws ConfigurationException
+    protected static URL getStorageConfigURL() throws ConfigurationException

Review Comment:
   looks like the tests do not use, can we go back to private?



##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -519,6 +559,11 @@ private static void addReplacement(Class<?> klass,
             // by default deprecated is false
             this.deprecated = deprecated;
         }
+
+        public boolean isValueFormatReplacement()

Review Comment:
   this got moved to its own top-level class, so will be a merge conflict on rebase



##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -199,6 +239,7 @@ public static <T> T fromMap(Map<String,Object> map, boolean shouldCheck, Class<T
     {
         Constructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader());
         Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
+        verifyReplacements(replacements, map);

Review Comment:
   when you rebase you will find a new method
   
   ```
   public static <T> T updateFromMap(Map<String, ? extends Object> map, boolean shouldCheck, T obj)
   ```
   
   Can you add this same logic there as well?  replacement is there + same style map, so just need this one line; cool doing this on merge



-- 
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