[GitHub] [pinot] klsince commented on a diff in pull request #10785: Allow env var substibution for Pinot configs

["klsince (via GitHub)" <gi...@apache.org>]

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. Thoughts?
   
   ```
     @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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org