xiangfu0 commented on code in PR #12307: URL: https://github.com/apache/pinot/pull/12307#discussion_r1462291215
########## 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: This will break all existing changes right? Are env var like `PINOT_SERVER_xxxx` loaded? I would suggest to keep current code rename it to legacy, so current user won't fail for using `PINOT_` prefix. Then add new code to override based on the new prefix. ########## 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: This will break all existing changes right? Are env var like `PINOT_SERVER_xxxx` loaded to `pinot.server.xxx`? I would suggest to keep current code rename it to legacy, so current user won't fail for using `PINOT_` prefix. Then add new code to override based on the new prefix. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org