Jackie-Jiang commented on code in PR #13011:
URL: https://github.com/apache/pinot/pull/13011#discussion_r1581321928
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigUtils.java:
##########
@@ -35,45 +36,55 @@ private ConfigUtils() {
private static final Map<String, String> ENVIRONMENT_VARIABLES =
System.getenv();
/**
- * Apply environment variables to any given BaseJsonConfig.
+ * Apply system properties and environment variables to any given
BaseJsonConfig.
+ * Environment variables take precedence over system properties.
+ * Since the System properties are mutable, this method will read it at
runtime.
*
- * @return Config with environment variable applied.
+ * @return Config with both system properties and environment variables
applied.
*/
- public static <T extends BaseJsonConfig> T applyConfigWithEnvVariables(T
config) {
- return applyConfigWithEnvVariables(ENVIRONMENT_VARIABLES, config);
+ public static <T extends BaseJsonConfig> T
applyConfigWithEnvVariablesAndSystemProperties(T config) {
+ Map<String, String> combinedMap = new HashMap<>();
+ // Add all system properties to the map
+ System.getProperties().forEach((key, value) ->
combinedMap.put(String.valueOf(key), String.valueOf(value)));
Review Comment:
Is the overhead high to dynamically load system properties for each config
read?
##########
pinot-spi/src/test/java/org/apache/pinot/spi/config/ConfigUtilsTest.java:
##########
@@ -85,7 +85,7 @@ public void testIndexing() {
ImmutableMap.of("LOAD_MODE", "MMAP", "AWS_ACCESS_KEY",
"default_aws_access_key", "AWS_SECRET_KEY",
"default_aws_secret_key");
- indexingConfig = ConfigUtils.applyConfigWithEnvVariables(environment,
indexingConfig);
+ indexingConfig =
ConfigUtils.applyConfigWithEnvVariablesAndSystemProperties(environment,
indexingConfig);
Review Comment:
Shall we add some value to the system properties and test out the behavior?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
