Skip to content

[common] Make dolphinscheduler_env.sh work when start server#9726

Merged
zhongjiajie merged 4 commits intoapache:devfrom
zhongjiajie:b-env-not-work
Apr 25, 2022
Merged

[common] Make dolphinscheduler_env.sh work when start server#9726
zhongjiajie merged 4 commits intoapache:devfrom
zhongjiajie:b-env-not-work

Conversation

@zhongjiajie
Copy link
Copy Markdown
Member

@zhongjiajie zhongjiajie commented Apr 24, 2022 •
edited
Loading

  • Change dist tarball dolphinscheduler_env.sh location
    from bin/ to conf/, which users could finish their
    change configuration operation in one single directory.
    and we only need to add $DOLPHINSCHEDULER_HOME/conf
    when we start our sever instead of adding both
    $DOLPHINSCHEDULER_HOME/conf and $DOLPHINSCHEDULER_HOME/bin
  • Change the start.sh's path of dolphinscheduler_env.sh
  • Change the setting order of dolphinscheduler_env.sh
  • Change the related docs

close: #9727

@zhongjiajie
[common] Make dolphinscheduler_env.sh work
6fd04b8 
* Change dist tarball `dolphinscheduler_env.sh` location
  from `bin/` to `conf/`, which users could finish their
  change configuration operation in one single directory.
  and we only need to add `$DOLPHINSCHEDULER_HOME/conf`
  when we start our sever instead of adding both
  `$DOLPHINSCHEDULER_HOME/conf` and `$DOLPHINSCHEDULER_HOME/bin`
* Change the `start.sh`'s path of `dolphinscheduler_env.sh`
* Change the setting order of `dolphinscheduler_env.sh`
* Change the related docs
@zhongjiajie zhongjiajie added bug Something isn't working backend labels Apr 24, 2022
@zhongjiajie zhongjiajie self-assigned this Apr 24, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 24, 2022 •
edited
Loading

Codecov Report

Merging #9726 (d7d9837) into dev (a51b710) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##                dev    #9726   +/-   ##
=========================================
  Coverage     40.00%   40.01%           
+ Complexity     4473     4472    -1     
=========================================
  Files           835      835           
  Lines         33550    33573   +23     
  Branches       3710     3714    +4     
=========================================
+ Hits          13423    13433   +10     
- Misses        18881    18889    +8     
- Partials       1246     1251    +5
Impacted Files Coverage Δ
.../org/apache/dolphinscheduler/common/Constants.java 80.95% <ø> (ø)
...che/dolphinscheduler/common/utils/CommonUtils.java 28.00% <0.00%> (ø)
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) ⬇️
...r/api/service/impl/ProcessInstanceServiceImpl.java 60.10% <0.00%> (-0.12%) ⬇️
...e/dolphinscheduler/server/master/MasterServer.java 0.00% <0.00%> (ø)
...e/dolphinscheduler/server/worker/WorkerServer.java 0.00% <0.00%> (ø)
...lphinscheduler/service/task/TaskPluginManager.java 0.00% <0.00%> (ø)
...er/server/master/runner/FailoverExecuteThread.java 0.00% <0.00%> (ø)
...r/server/master/runner/MasterSchedulerService.java 0.00% <0.00%> (ø)
...nscheduler/service/process/ProcessServiceImpl.java 30.75% <0.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51b710...d7d9837. Read the comment docs.

@zhongjiajie zhongjiajie changed the title [common] Make dolphinscheduler_env.sh work [common] Make dolphinscheduler_env.sh work when start server Apr 24, 2022
kezhenxu94
kezhenxu94 previously approved these changes Apr 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM overall

dolphinscheduler-master/src/main/bin/start.sh Outdated
DOLPHINSCHEDULER_HOME=${DOLPHINSCHEDULER_HOME:-$(cd $BIN_DIR/..; pwd)}

source "$BIN_DIR/dolphinscheduler_env.sh"
source "$BIN_DIR/../conf/dolphinscheduler_env.sh"
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 Apr 24, 2022

Choose a reason for hiding this comment

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

What about just

Suggested change
source "$BIN_DIR/../conf/dolphinscheduler_env.sh"
source "$DOLPHINSCHEDULER_HOME/conf/dolphinscheduler_env.sh"

Copy link
Copy Markdown
Member Author

@zhongjiajie zhongjiajie Apr 25, 2022

Choose a reason for hiding this comment

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

Will change it

Copy link
Copy Markdown
Member Author

@zhongjiajie zhongjiajie Apr 25, 2022

Choose a reason for hiding this comment

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

Done and could you please take a look again? BTW, why we should add file dolphinscheduler_env.sh in all servers instead of using the file in the path bin/env/dolphinscheduler_env.sh?

Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 Apr 25, 2022

Choose a reason for hiding this comment

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

BTW, why we should add file dolphinscheduler_env.sh in all servers instead of using the file in the path bin/env/dolphinscheduler_env.sh?

See my comment #9675 (comment) . The short answer is that bin/env/dolphinscheduler_env.sh is only available when you use install.sh to install all components in a single machine. If you install each component in a separate machine, bin/env/dolphinscheduler_env.sh is not available there, also cc @SbloodyS

@zhongjiajie zhongjiajie requested a review from kezhenxu94 April 25, 2022 01:41
@SbloodyS
Copy link
Copy Markdown
Member

SbloodyS commented Apr 25, 2022 •
edited
Loading

I have a suggestion in #9675 . Because I found that many PR and issues are related to this problem. If everyone agrees, I think we can add it to this PR to avoid duplication of work. Then close other related issues.

Please task a look @zhongjiajie @kezhenxu94 @caishunfeng

@SbloodyS SbloodyS mentioned this pull request Apr 25, 2022
Closed
3 tasks
kezhenxu94
kezhenxu94 previously approved these changes Apr 25, 2022
View reviewed changes
@zhongjiajie
dolphinscheduler_env.sh overwrite when start with shell in bin dir
8a2a1cc 
`bin/env/dolphinscheduler_env.sh` will overwrite the `<server>/conf/dolphinscheduler_env.sh`
when start the server using `bin/dolphinsceduler_daemon.sh` or `bin/install.sh`
@zhongjiajie
Copy link
Copy Markdown
Member Author

zhongjiajie commented Apr 25, 2022

I add feature we metiond in #9675 (comment), and after this patch merged, user s who using commad ./bin/dolphinscheduler_daemon.sh start <server> or ./bin/install.sh to start dolphinscheduler, the server env config will be overwrite by ./bin/env/dolphinscheudler_env.sh.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@zhongjiajie
Copy link
Copy Markdown
Member Author

zhongjiajie commented Apr 25, 2022

I going to merge this one

@zhongjiajie zhongjiajie merged commit de50f43 into apache:dev Apr 25, 2022
@zhongjiajie zhongjiajie deleted the b-env-not-work branch April 25, 2022 07:35
@zhongjiajie zhongjiajie mentioned this pull request Apr 26, 2022
Closed
3 tasks
@EricGao888 EricGao888 mentioned this pull request Apr 26, 2022
Merged
Tianqi-Dotes pushed a commit to Tianqi-Dotes/dolphinscheduler that referenced this pull request Jun 16, 2022
@zhongjiajie
[common] Make dolphinscheduler_env.sh work when start server (apache#…
21596ef 
…9726)

* [common] Make dolphinscheduler_env.sh work

* Change dist tarball `dolphinscheduler_env.sh` location
  from `bin/` to `conf/`, which users could finish their
  change configuration operation in one single directory.
  and we only need to add `$DOLPHINSCHEDULER_HOME/conf`
  when we start our sever instead of adding both
  `$DOLPHINSCHEDULER_HOME/conf` and `$DOLPHINSCHEDULER_HOME/bin`
* Change the `start.sh`'s path of `dolphinscheduler_env.sh`
* Change the setting order of `dolphinscheduler_env.sh`
* `bin/env/dolphinscheduler_env.sh` will overwrite the `<server>/conf/dolphinscheduler_env.sh`
when start the server using `bin/dolphinsceduler_daemon.sh` or `bin/install.sh`
* Change the related docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SbloodyS SbloodyS SbloodyS approved these changes

@kezhenxu94 kezhenxu94 kezhenxu94 left review comments

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

Assignees

@zhongjiajie zhongjiajie

Labels

backend bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] [common] dolphinscheduler_env.sh not work even though configured

4 participants

@zhongjiajie @codecov-commenter @SbloodyS @kezhenxu94