-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[hotfix-simple][#fix-bin]#fix-commit #1187
Conversation
Also, you need to squeeze your commits into one before we finally merge this request. |
bin/submit.sh
Outdated
@@ -34,9 +34,9 @@ fi | |||
if [[ $CHUNJUN_HOME && -z $CHUNJUN_HOME ]];then | |||
export CHUNJUN_HOME=$CHUNJUN_HOME | |||
else | |||
export CHUNJUN_HOME="$(cd "`dirname "$0"`"/../chunjun-dist; pwd)" | |||
export CHUNJUN_HOME="$(cd "`dirname "$0"`"/..; pwd)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, your pr is considered the situation that CHUNJUN_HOME is chunjun-dist, like
ls chunjun-dist
bin connector dirty-data-collector formats metrics
chunjun-core-master.jar ddl docker-build lib restore-plugins
which means that user use chunjun from release package.
But we also need to consider the situation that CHUNJUN_HOME is chunjun(project path, parent path of chunjun-dist), like
ls chunjun
CONTRIBUTING.md chunjun-assembly chunjun-dist chunjun-restore flinkx-core lib
LICENSE chunjun-clients chunjun-docker chunjun-sql flinkx-dirtydata-collectors mvnw
README.md chunjun-connectors chunjun-examples chunjun.iml flinkx-docker mvnw.cmd
README_CH.md chunjun-core chunjun-formats docs_zh flinkx-formats pom.xml
bin chunjun-dev chunjun-local-test flinkx-clients flinkx-metrics website
build chunjun-dirty chunjun-metrics flinkx-connectors flinkx-restore
which means that user use chunjun from Git clone.
So you need to consider this two situations using 'if' statement.
JAR_DIR=$CHUNJUN_HOME/lib/* | ||
elif [ $CHUNJUN_DEPLOY_MODE -eq 2 ]; then | ||
JAR_DIR=$CHUNJUN_HOME/../lib/* | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two else case can be merged into one.
Look good to me. |
Purpose of this pull request
fix the problem1: when invoking a starting script such as chunjun-yarn-perjob.sh with the correct CHUNJUN_HOME env var, chunjun program fails cause "cd XXX/../chunjun_dist not found"
fix the problem2: when invoking a starting script such as chunjun-yarn-perjob.sh with the correct CHUNJUN_HOME env var, chunjun program can not load right classpath because of the wrong classpath path setting "xxx/chunjun-dist/../lib“
Which issue you fix
Fixes # (issue).
Checklist: