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

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

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


##########
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?
   
   Thought about it. In general it seems good to me. But I wonder how many more configurations would be loaded from env. I referenced Spark, which doesn't have this `loadFromEnv` option..
   
   How about we keep this a while, if there are two/three more configurations that should be read from env, then let's refactor and add the feature for ConfigBuilder?



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