klsince commented on code in PR #10785:
URL: https://github.com/apache/pinot/pull/10785#discussion_r1201214212
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java:
##########
@@ -119,8 +120,56 @@ public static Map<String, Object> toMap(Configuration
configuration) {
}
private static Object mapValue(String key, Configuration configuration) {
- return Optional.of(configuration.getStringArray(key)).filter(values ->
values.length > 1).<Object>map(
- values -> Arrays.stream(values).collect(Collectors.joining(",")))
- .orElseGet(() -> configuration.getProperty(key));
+ // For multi-value config, convert it to a single comma connected string
value.
+ // For single-value config, return its raw property, unless it needs
interpolation.
+ return Optional.of(configuration.getStringArray(key)).filter(values ->
values.length > 1)
+ .<Object>map(values ->
Arrays.stream(values).collect(Collectors.joining(","))).orElseGet(() -> {
+ Object rawProperty = configuration.getProperty(key);
+ if (!needInterpolate(rawProperty)) {
+ return rawProperty;
+ }
+ // The string value is converted to the requested type when
accessing it via PinotConfiguration.
+ return configuration.getString(key);
+ });
+ }
+
+ public static boolean needInterpolate(Object rawProperty) {
+ if (rawProperty instanceof String) {
+ String strProperty = (String) rawProperty;
+ // e.g. if config value is '${sys:ENV_VAR_FOO}', it's replaced by value
of env var ENV_VAR_FOO.
Review Comment:
sharp eye. updated the comment, and this reminded me to test env var, other
than system property. The change in this PR can interpolate with env var as
well, but I find it non-trivial to add a unit test for env var.
There seems some specific temporal behavior when CompositeConfiguration
interpolates with env vars, thus running the unit test below alone was fine,
but failed if running with other test cases. It seems after the first call of
`pinotConfiguration.getProperty()`, the interpolation with env vars was done
and wouldn't be done any more. Depending on if `getProperty()` or those
`setEnv()` was called firstly, the unit test could fail. This should not be an
issue in real prod, as all env vars are loaded firstly when JVM starts.
```
@Test
public void assertEnvVarInterpolation()
throws Exception {
Map<String, Object> configs = new HashMap<>();
for (int i = 0; i <= 3; i++) {
configs.put("config.property." + i,
"${env:PINOT_CONFIGURATION_TEST_VAR" + i + "}");
}
configs.put("config.property.4",
"${env:PINOT_CONFIGURATION_TEST_VAR4},${env:PINOT_CONFIGURATION_TEST_VAR5}");
PinotConfiguration pinotConfiguration = new PinotConfiguration(configs);
try {
// 1. not like System property, the value of env var can't be updated
once defined, so define them ahead.
// 2. the interpolation seems like happening once when the first of
pinotConfiguration.getProperty() is called
setEnv("PINOT_CONFIGURATION_TEST_VAR1", "val1");
setEnv("PINOT_CONFIGURATION_TEST_VAR2", "true");
setEnv("PINOT_CONFIGURATION_TEST_VAR3", "10");
setEnv("PINOT_CONFIGURATION_TEST_VAR4", "a");
setEnv("PINOT_CONFIGURATION_TEST_VAR5", "b");
// Env var 0 is not defined, thus interpolation doesn't happen
Assert.assertEquals(pinotConfiguration.getProperty("config.property.0"),
"${env:PINOT_CONFIGURATION_TEST_VAR0}");
// String value
Assert.assertEquals(pinotConfiguration.getProperty("config.property.1"),
"val1");
Assert.assertEquals(pinotConfiguration.getProperty("config.property.1",
"defaultVal"), "val1");
Map<String, Object> properties = pinotConfiguration.toMap();
Assert.assertEquals(properties.get("config.property.1"), "val1");
// Boolean value
Assert.assertTrue(pinotConfiguration.getProperty("config.property.2",
false));
properties = pinotConfiguration.toMap();
Assert.assertEquals(properties.get("config.property.2"), "true");
Assert.assertTrue(new
PinotConfiguration(properties).getProperty("config.property.2", false));
// Number value
Assert.assertEquals(pinotConfiguration.getProperty("config.property.3", 0), 10);
properties = pinotConfiguration.toMap();
Assert.assertEquals(properties.get("config.property.3"), "10");
Assert.assertEquals(new
PinotConfiguration(properties).getProperty("config.property.3", 0), 10);
// String array, with elements specified as env vars separately.
pinotConfiguration = new PinotConfiguration(configs);
Assert.assertEquals(pinotConfiguration.getProperty("config.property.4",
Arrays.asList()),
Arrays.asList("a", "b"));
properties = pinotConfiguration.toMap();
Assert.assertEquals(properties.get("config.property.4"), "a,b");
Assert.assertEquals(new
PinotConfiguration(properties).getProperty("config.property.4",
Arrays.asList()),
Arrays.asList("a", "b"));
} finally {
for (int i = 0; i <= 5; i++) {
clearEnv("PINOT_CONFIGURATION_TEST_VAR" + i);
}
}
}
```
--
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]
