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


##########
bin/utils.sh:
##########
@@ -159,6 +159,12 @@ function load_rss_env {
 
   # find rss-env.sh
   set +o nounset
+  if [ -z "$RSS_HOME" ]; then
+    RSS_HOME="$(cd "$(dirname "$0")/.."; pwd)"
+  fi
+  if [ -z "$RSS_CONF_DIR" ]; then
+    RSS_CONF_DIR="${RSS_HOME}/conf"
+  fi

Review Comment:
   I'm wondering the implication of these changes.
   
   If RSS_HOME/RSS_CONF_DIR is both set in rss-env.sh and manually set by user 
when starting the scripts, there's a  chance that they are different, and the 
settings in rss-env.sh take preference over manually set by user. I think that 
might be confusing and error-prone. Let's add some clarification in the 
rss-env.sh, something like:
   
   ```shell
   # RSS_HOME, RSS home directory (Default: parent directory of the script).
   # If you want to set it to another place, please make sure the RSS_HOME
   # specified externally is respected, by defining it as follows:
   #
   # RSS_HOME=${RSS_HOME:-{another_rss_home_path}}
   # 
   # RSS_CONF_DIR, RSS configuration directory (Default: ${RSS_HOME}/conf)
   # Similar with RSS_HOME, RSS_CONF_DIR should respect external env variable.
   # 
   # RSS_CONF_DIR=${RSS_CONF_DIR:-{another_rss_conf_dir}}
   ```



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to