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

Reply via email to