smiklosovic commented on code in PR #4231:
URL: https://github.com/apache/cassandra/pull/4231#discussion_r2256672681
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -180,6 +180,7 @@ public enum CassandraRelevantProperties
* Default is set to false.
*/
COM_SUN_MANAGEMENT_JMXREMOTE_SSL_NEED_CLIENT_AUTH("com.sun.management.jmxremote.ssl.need.client.auth"),
+
CONFIG_ALLOW_ENVIRONMENT_VARIABLES("cassandra.config.allow_environment_variables"),
Review Comment:
why don't we also have environment property for this? In
`CassandraRelevantEnv`, you would have
`CASSANDRA_ALLOW_CONFIG_ENVIRONMENT_VARIABLES("CASSANDRA_ALLOW_CONFIG_ENVIRONMENT_VARIABLES")`
System property would override environment one.
Basically you would specify if environment properties might be set by
reacting on an environment property itself.
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -72,6 +74,9 @@ public class YamlConfigurationLoader implements
ConfigurationLoader
* system properties do not conflict with other system properties; the
name "settings" matches system_views.settings.
*/
static final String SYSTEM_PROPERTY_PREFIX = "cassandra.settings.";
+ static final String ENVIRONMENT_VARIABLE_PREFIX = "CASS_";
Review Comment:
I would do `CASSANDRA_SETTINGS_` here (or at least `CASSANDRA_`. If we have
`cassandra.settings` instead of `cass.settings`, why we would introduce this
discrepancy just to save few characters ... .
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -238,6 +286,7 @@ public static <T> T fromMap(Map<String,Object> map, boolean
shouldCheck, Class<T
if (shouldCheck)
propertiesChecker.check();
maybeAddSystemProperties(value);
+ maybeAddEnvironmentVariables(value);
Review Comment:
I would enable this, yes.
##########
src/java/org/apache/cassandra/config/YamlConfigurationLoader.java:
##########
@@ -163,21 +169,63 @@ private static void maybeAddSystemProperties(Object obj)
if
(CassandraRelevantProperties.CONFIG_ALLOW_SYSTEM_PROPERTIES.getBoolean())
{
java.util.Properties props = System.getProperties();
- Map<String, String> map = new HashMap<>();
- for (String name : props.stringPropertyNames())
+ Map<String, Object> map = new HashMap<>();
+ for (String originalKey : props.stringPropertyNames())
{
- if (name.startsWith(SYSTEM_PROPERTY_PREFIX))
+ if (originalKey.startsWith(SYSTEM_PROPERTY_PREFIX))
{
- String value = props.getProperty(name);
- if (value != null)
- map.put(name.replace(SYSTEM_PROPERTY_PREFIX, ""),
value);
+ String value = props.getProperty(originalKey);
+ String configKey =
originalKey.replace(SYSTEM_PROPERTY_PREFIX, "");
+ if (value != null && !map.containsKey(configKey)) {
+ if (!DatabaseDescriptor.hasLoggedConfig()) //
CASSANDRA-9909: Avoid flooding config during initialization
+ logger.warn("Detected JVM property {}={} override
for cassandra configuration '{}' (ignored if setting does not exist).",
originalKey, value, configKey);
+ map.put(configKey, getScalarValueOrJsonObject(value));
+ }
}
}
if (!map.isEmpty())
updateFromMap(map, false, obj);
}
}
+ private static void maybeAddEnvironmentVariables(Object obj)
+ {
+ if
(CassandraRelevantProperties.CONFIG_ALLOW_ENVIRONMENT_VARIABLES.getBoolean())
Review Comment:
I have suggested this above. I would introduce also environment property for
this feature which sets _environment properties_.
We can introduce both system and env enabling property and when system
enabling property is set it would override what env enabling property is set
to, so system property would have precedence.
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
