[GitHub] [incubator-uniffle] zuston commented on a diff in pull request #555: [#554] infer rss base storage conf from env

["zuston (via GitHub)" <gi...@apache.org>]

zuston commented on code in PR #555:
URL: https://github.com/apache/incubator-uniffle/pull/555#discussion_r1098230745


##########
common/src/main/java/org/apache/uniffle/common/util/RssUtils.java:
##########
@@ -292,4 +296,12 @@ public static Roaring64NavigableMap generateTaskIdBitMap(Roaring64NavigableMap b
     }
     return taskIdBitmap;
   }
+
+  public static List<String> getConfiguredLocalDirs(RssConf conf) {

Review Comment:
   Could we have a general way to support conf value from env instead of supporting this one by one? 



##########
common/src/main/java/org/apache/uniffle/common/util/RssUtils.java:
##########
@@ -292,4 +296,12 @@ public static Roaring64NavigableMap generateTaskIdBitMap(Roaring64NavigableMap b
     }
     return taskIdBitmap;
   }
+
+  public static List<String> getConfiguredLocalDirs(RssConf conf) {

Review Comment:
   For example: marking the config, whose value could be from env. Like that
   
   ```
   public static final ConfigOption<Long> SERVER_BUFFER_CAPACITY = ConfigOptions
         .key("rss.server.buffer.capacity")
         .longType()
         .noDefaultValue()
         .allowFromEnv()
         .withDescription("Max memory of buffer manager for shuffle server");
   ```
   
   Once one config option is allowed from env, it will be initialized from env when creating the `shuffleServerConf`. WDYT? 



-- 
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: issues-unsubscribe@uniffle.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org