You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "klsince (via GitHub)" <gi...@apache.org> on 2023/05/19 18:41:17 UTC
[GitHub] [pinot] klsince opened a new pull request, #10785: [WIP] allow env var substibution for Pinot configs
klsince opened a new pull request, #10785:
URL: https://github.com/apache/pinot/pull/10785
Testing some ideas to make env var substitution work for Pinot configs
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10785: [WIP] allow env var substibution for Pinot configs
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10785:
URL: https://github.com/apache/pinot/pull/10785#discussion_r1199417840
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -357,6 +358,9 @@ public long getProperty(String name, long defaultValue) {
*/
public String getProperty(String name, String defaultValue) {
Object rawProperty = getRawProperty(name, defaultValue);
Review Comment:
yeah, why not. lemme update
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10785: Allow env var substibution for Pinot configs
Posted by "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.
```
@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
[GitHub] [pinot] Jackie-Jiang merged pull request #10785: Allow env var substibution for Pinot configs
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10785:
URL: https://github.com/apache/pinot/pull/10785
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10785: Allow env var substibution for Pinot configs
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10785:
URL: https://github.com/apache/pinot/pull/10785#discussion_r1203118997
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -356,7 +356,14 @@ public long getProperty(String name, long defaultValue) {
* @return the property String value. Fallback to default value if missing.
*/
public String getProperty(String name, String defaultValue) {
- Object rawProperty = getRawProperty(name, defaultValue);
+ String relaxedPropertyName = relaxPropertyName(name);
+ if (!_configuration.containsKey(relaxedPropertyName)) {
+ return defaultValue;
+ }
+ Object rawProperty = _configuration.getProperty(relaxedPropertyName);
+ if (CommonsConfigurationUtils.needInterpolate(rawProperty)) {
+ return _configuration.getString(relaxPropertyName(name), defaultValue);
+ }
if (rawProperty instanceof List) {
return StringUtils.join(((ArrayList) rawProperty).toArray(), ',');
Review Comment:
fixed this according to an idea proposed by @dd-willgan in a comment early on. I didn't get it but now I see what's going on with multi-value interpolation. I expanded unit tests for such case. pls review.
--
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
[GitHub] [pinot] dd-willgan commented on a diff in pull request #10785: [WIP] allow env var substibution for Pinot configs
Posted by "dd-willgan (via GitHub)" <gi...@apache.org>.
dd-willgan commented on code in PR #10785:
URL: https://github.com/apache/pinot/pull/10785#discussion_r1199344351
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -357,6 +358,9 @@ public long getProperty(String name, long defaultValue) {
*/
public String getProperty(String name, String defaultValue) {
Object rawProperty = getRawProperty(name, defaultValue);
Review Comment:
For this couldn't we check if the property key exists and if not use defaultValue, otherwise just have the same logic as getProperty(String name, Configuration configuration)? That would handle the multi-value case I think
--
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
[GitHub] [pinot] dd-willgan commented on a diff in pull request #10785: Allow env var substibution for Pinot configs
Posted by "dd-willgan (via GitHub)" <gi...@apache.org>.
dd-willgan commented on code in PR #10785:
URL: https://github.com/apache/pinot/pull/10785#discussion_r1200790730
##########
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:
`${env:ENV_VAR_FOO}` would be replaced by env var ENV_VAR_FOO?
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10785: Allow env var substibution for Pinot configs
Posted by "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
[GitHub] [pinot] codecov-commenter commented on pull request #10785: [WIP] allow env var substibution for Pinot configs
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10785:
URL: https://github.com/apache/pinot/pull/10785#issuecomment-1555127341
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10785?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10785](https://app.codecov.io/gh/apache/pinot/pull/10785?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8bd2bf3) into [master](https://app.codecov.io/gh/apache/pinot/commit/efb4f46775490d2a2179a235ac42bf8c0ba37823?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (efb4f46) will **decrease** coverage by `56.64%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10785 +/- ##
=============================================
- Coverage 70.32% 13.68% -56.64%
+ Complexity 5658 439 -5219
=============================================
Files 2159 2104 -55
Lines 116028 113572 -2456
Branches 17563 17278 -285
=============================================
- Hits 81593 15541 -66052
- Misses 28738 96757 +68019
+ Partials 5697 1274 -4423
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.68% <0.00%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...pache/pinot/spi/env/CommonsConfigurationUtils.java](https://app.codecov.io/gh/apache/pinot/pull/10785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52L0NvbW1vbnNDb25maWd1cmF0aW9uVXRpbHMuamF2YQ==) | `0.00% <0.00%> (-65.39%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/env/PinotConfiguration.java](https://app.codecov.io/gh/apache/pinot/pull/10785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52L1Bpbm90Q29uZmlndXJhdGlvbi5qYXZh) | `0.00% <0.00%> (-91.77%)` | :arrow_down: |
... and [1710 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10785/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10785: Allow env var substibution for Pinot configs
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10785:
URL: https://github.com/apache/pinot/pull/10785#discussion_r1203041739
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -356,7 +356,14 @@ public long getProperty(String name, long defaultValue) {
* @return the property String value. Fallback to default value if missing.
*/
public String getProperty(String name, String defaultValue) {
- Object rawProperty = getRawProperty(name, defaultValue);
+ String relaxedPropertyName = relaxPropertyName(name);
+ if (!_configuration.containsKey(relaxedPropertyName)) {
+ return defaultValue;
+ }
+ Object rawProperty = _configuration.getProperty(relaxedPropertyName);
+ if (CommonsConfigurationUtils.needInterpolate(rawProperty)) {
+ return _configuration.getString(relaxPropertyName(name), defaultValue);
Review Comment:
```suggestion
return _configuration.getString(relaxedPropertyName, defaultValue);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -372,8 +379,11 @@ private <T> T getProperty(String name, T defaultValue, Class<T> returnType) {
if (!_configuration.containsKey(relaxedPropertyName)) {
return defaultValue;
}
-
- return PropertyConverter.convert(getRawProperty(name, defaultValue), returnType);
+ Object rawProperty = _configuration.getProperty(relaxedPropertyName);
+ if (CommonsConfigurationUtils.needInterpolate(rawProperty)) {
+ return CommonsConfigurationUtils.interpolate(_configuration, relaxPropertyName(name), defaultValue, returnType);
Review Comment:
```suggestion
return CommonsConfigurationUtils.interpolate(_configuration, relaxedPropertyName, defaultValue, returnType);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -356,7 +356,14 @@ public long getProperty(String name, long defaultValue) {
* @return the property String value. Fallback to default value if missing.
*/
public String getProperty(String name, String defaultValue) {
- Object rawProperty = getRawProperty(name, defaultValue);
+ String relaxedPropertyName = relaxPropertyName(name);
+ if (!_configuration.containsKey(relaxedPropertyName)) {
+ return defaultValue;
+ }
+ Object rawProperty = _configuration.getProperty(relaxedPropertyName);
+ if (CommonsConfigurationUtils.needInterpolate(rawProperty)) {
+ return _configuration.getString(relaxPropertyName(name), defaultValue);
+ }
if (rawProperty instanceof List) {
return StringUtils.join(((ArrayList) rawProperty).toArray(), ',');
Review Comment:
Do we need to interpolate here? Or do we need to handle List when reading string?
--
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