xiangfu0 commented on code in PR #12307:
URL: https://github.com/apache/pinot/pull/12307#discussion_r1463922140
##########
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java:
##########
@@ -201,16 +200,16 @@ private static Configuration loadProperties(String
configPath) {
private static Map<String, Object> relaxConfigurationKeys(Configuration
configuration) {
return CommonsConfigurationUtils.getKeysStream(configuration).distinct()
- .collect(Collectors.toMap(PinotConfiguration::relaxPropertyName, key
-> configuration.getProperty(key)));
+ .collect(Collectors.toMap(PinotConfiguration::relaxPropertyName,
configuration::getProperty));
}
private static Map<String, String> relaxEnvironmentVariables(Map<String,
String> environmentVariables) {
- return environmentVariables.entrySet().stream().filter(entry ->
entry.getKey().startsWith("PINOT_"))
+ return environmentVariables.entrySet().stream().filter(entry ->
entry.getKey().startsWith(ENV_PREFIX))
Review Comment:
1.
For the backward compatible perspective, I think the issue here is that for
whoever currently using `PINOT_CONTROLLER_HOST` will see the unexpected
behavior the override won't happen.
Suggest to rename existing `relaxEnvVarName` to `legacyRelaxEnvVarName`, and
have your new `relaxEnvVarName` to override.
E.g. existing code:
`PINOT_CONTROLLER_HOST` should be added as `controller.host`;
`PINOT_PINOT_CONTROLLER_HOST` should be added as `pinot.controller.host`;
`PINOT_ENV_SERVER_XXX` should be added as `server.xxx`;
`PINOT_ENV_PINOT_SERVER_XXX` should be added as `pinot.server.xxx`;
2.
From the ease of use perspective, actually I would love to have the direct
mapping from env variable to configs, e.g.
`pinot.server.xxx` should be override by `PINOT_SERVER_XXX`.
But due to some legacy issue that not all configs are start with `pinot.`
that's why we introduce this `PINOT_` prefix.
IMO, I would love to map env var `PINOT_XXX_YYY` to both `xxx.yyy` and
`pinot.xxx.yyy`.
So that we can cover both.
Just for `xxx.yyy` may override existing configs but `pinot.xxx.yyy` will be
added if not existed.
cc: @Jackie-Jiang @snleee @siddharthteotia @chenboat would love your best
practice of config override.
--
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]
