[GitHub] [bookkeeper] mino181295 commented on a change in pull request #2347: ISSUE-2341 Load bookkeeper parameters from environment variables

[GitBox <gi...@apache.org>]

mino181295 commented on a change in pull request #2347:
URL: https://github.com/apache/bookkeeper/pull/2347#discussion_r429674568



##########
File path: bookkeeper-server/pom.xml
##########
@@ -170,6 +170,11 @@
       <version>${project.parent.version}</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>com.github.stefanbirkner</groupId>
+      <artifactId>system-rules</artifactId>
+      <scope>test</scope>

Review comment:
       nit: Better have this in the main pom?

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/server/Main.java
##########
@@ -112,6 +112,16 @@ private static void loadConfFile(ServerConfiguration conf, String confFile)
         log.info("Using configuration file {}", confFile);
     }
 
+    private static void loadConfEnv(ServerConfiguration conf) throws IllegalArgumentException {

Review comment:
       What about this in `LocalBookKeeper` ?

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/server/Main.java
##########
@@ -125,6 +135,8 @@ private static ServerConfiguration parseArgs(String[] args)
 
             ServerConfiguration conf = new ServerConfiguration();
 
+            loadConfEnv(conf);
+
             if (cmdLine.hasOption('c')) {
                 String confFile = cmdLine.getOptionValue("c");
                 loadConfFile(conf, confFile);

Review comment:
       Do you expect `loadConfFile` overrides `loadConfFile` if the same property is defined?




----------------------------------------------------------------
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.

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