Optimize the bash script #29
Conversation
|
@jerqi Could u help review this? I found the current deploy script is not compatible with our internal Hadoop distribution. |
conf/log4j.shuffle_server.properties
Outdated
| log4j.appender.console.layout=org.apache.log4j.PatternLayout | ||
| log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n | ||
| log4j.appender.RollingAppender=org.apache.log4j.RollingFileAppender | ||
| log4j.appender.RollingAppender.File=./logs/shuffle_server.log |
There was a problem hiding this comment.
Could we introduce a environment variable like LOG_PATH in start script?
In log4j.properties, we use the environment variable to help coordinator and server to write log in different paths.
There was a problem hiding this comment.
In my test cases, i make the coordinator and shuffle server deployed to one node. So i think it's better to separate the log file.
Although the log file content looks same now, but separation will make log content change more easily.
There was a problem hiding this comment.
First, We don't recommend to deploy coordinator and server in one node.
If we deploy multiple servers in one node, do we need configurations conf/log4j.shuffle_server.properties.1,conf/log4j.shuffle_server.properties.2?
Second, Hadoop DataNode and Hadoop NodeManager can be deployed in one node, but they share one log4j.properties.
There was a problem hiding this comment.
Ok. Got it.
Besides, why we dont recommend to deploy in one node? Can u share some info? As i know, the coordinator is lightweight
There was a problem hiding this comment.
If there are too many applications, the server will occupy all the network bandwidth and coordinator will be unstable.
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
============================================
- Coverage 56.83% 56.80% -0.04%
+ Complexity 1204 1203 -1
============================================
Files 152 152
Lines 8401 8401
Branches 813 813
============================================
- Hits 4775 4772 -3
- Misses 3368 3370 +2
- Partials 258 259 +1
Continue to review full report at Codecov.
|
| echo "No env HADOOP_HOME." | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
I verify this patch offline, find this error
When I add
export JAVA_HOME
here.
It's ok.
There was a problem hiding this comment.
JAVA_HOME has been set in rss-env.sh.
I can't reproduce it.
bin/start-coordinator.sh
Outdated
| LOG_CONF_FILE="./conf/log4j.properties" | ||
| LOG_PATH="./logs/coordinator.log" | ||
| if [ -f ${LOG_CONF_FILE} ]; then | ||
| ARGS="$ARGS -Dlog4j.configuration=file:${LOG_CONF_FILE} -DLOG_PATH=${LOG_PATH}" |
There was a problem hiding this comment.
-Dlog.path=${LOG_PATH} may be better. The style will be consistent with -Dlog4j.configuration.
conf/log4j.properties
Outdated
| @@ -22,7 +22,7 @@ log4j.appender.console.target=System.err | |||
| log4j.appender.console.layout=org.apache.log4j.PatternLayout | |||
| log4j.appender.console.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n | |||
| log4j.appender.RollingAppender=org.apache.log4j.RollingFileAppender | |||
| log4j.appender.RollingAppender.File=./logs/rss.log | |||
| log4j.appender.RollingAppender.File=${LOG_PATH} | |||
There was a problem hiding this comment.
If we use log.path, we should modify this place, too.
What changes were proposed in this pull request?
Optimzie the bash script when deploying.
Main changelog
bin/hadoopto get the hadoop deps.Why are the changes needed?
To be compatible with the other hadoop distribution Including Apache Hadoop, like CDH.
Does this PR introduce any user-facing change?
Yes.
How was this patch tested?
Manual test.