Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improvement] Add optional environment variables #187

Merged
merged 2 commits into from Aug 29, 2022
Merged

Conversation

izchen
Copy link
Contributor

@izchen izchen commented Aug 25, 2022

What changes were proposed in this pull request?

Added six optional environment variables:

# RSS_HOME, RSS home directory (Default: parent directory of the script)
# RSS_CONF_DIR, RSS configuration directory (Default: ${RSS_HOME}/conf)
# HADOOP_CONF_DIR, Hadoop configuration directory (Default: ${HADOOP_HOME}/etc/hadoop)
# RSS_PID_DIR, Where the pid file is stored (Default: ${RSS_HOME})
# RSS_LOG_DIR, Where log files are stored (Default: ${RSS_HOME}/logs)
# RSS_IP, IP address Shuffle Server binds to on this node (Default: first non-loopback ipv4)

Why are the changes needed?

Simplify service deployment

Does this PR introduce any user-facing change?

Added six optional environment variables.

With the default environment variables, the behavior is the same as before this PR.

How was this patch tested?

local test

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2022

Codecov Report

Merging #187 (bf81a4b) into master (5864420) will increase coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #187      +/-   ##
============================================
+ Coverage     58.37%   58.45%   +0.07%     
- Complexity     1270     1272       +2     
============================================
  Files           158      158              
  Lines          8428     8437       +9     
  Branches        782      782              
============================================
+ Hits           4920     4932      +12     
+ Misses         3255     3254       -1     
+ Partials        253      251       -2     
Impacted Files Coverage Δ
.../apache/uniffle/coordinator/CoordinatorServer.java 64.89% <0.00%> (-0.70%) ⬇️
.../java/org/apache/uniffle/server/ShuffleServer.java 68.42% <0.00%> (-0.52%) ⬇️
.../org/apache/uniffle/common/config/RssBaseConf.java 93.42% <0.00%> (+0.27%) ⬆️
...org/apache/uniffle/server/ShuffleFlushManager.java 79.32% <0.00%> (+1.67%) ⬆️
...uniffle/common/security/HadoopSecurityContext.java 81.57% <0.00%> (+5.90%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@colinmjj
Copy link

How about unify COORDINATOR_HOME and SHUFFLE_SERVER_HOME to RSS_HOME
All new environment variables can be defined in rss-env.sh

@izchen
Copy link
Contributor Author

izchen commented Aug 25, 2022

How about unify COORDINATOR_HOME and SHUFFLE_SERVER_HOME to RSS_HOME
All new environment variables can be defined in rss-env.sh

I think we can define RSS_HOME and RSS_CONF_DIR in rss-env.sh. I will modify the script later.

@@ -20,10 +20,8 @@
set -o pipefail
set -o nounset # exit the script if you try to use an uninitialised variable
set -o errexit # exit the script if any statement returns a non-true return value
set -e
Copy link
Contributor Author

@izchen izchen Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unify style. In these scripts, rewrite set -e to set -o errexit.

set -e same as set -o errexit: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html

@izchen
Copy link
Contributor Author

izchen commented Aug 29, 2022

How about unify COORDINATOR_HOME and SHUFFLE_SERVER_HOME to RSS_HOME
All new environment variables can be defined in rss-env.sh

Done.

Would you mind take a look when you have some time? @colinmjj

@colinmjj
Copy link

LGTM, +1, thanks for the contribution, @izchen

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1, @izchen thanks

@jerqi jerqi merged commit 6be4ee0 into apache:master Aug 29, 2022
@izchen izchen deleted the rss_env branch August 29, 2022 05:44
@izchen
Copy link
Contributor Author

izchen commented Aug 29, 2022

Thanks for your review, @colinmjj @jerqi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants