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

[Bug][Tools] Add datasource url for mysql profile in tools application.yaml #10399

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

EricGao888
Copy link
Member

Purpose of the pull request

Brief change log

  • Already described above.

Verify this pull request

  • Verified by manual test.

@codecov-commenter
Copy link

Codecov Report

Merging #10399 (aa7cc0c) into dev (3258438) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #10399      +/-   ##
============================================
+ Coverage     40.56%   40.57%   +0.01%     
- Complexity     4766     4769       +3     
============================================
  Files           877      877              
  Lines         35621    35621              
  Branches       3945     3945              
============================================
+ Hits          14449    14454       +5     
+ Misses        19737    19733       -4     
+ Partials       1435     1434       -1     
Impacted Files Coverage Δ
...r/plugin/task/sqoop/parameter/SqoopParameters.java 53.33% <0.00%> (ø)
...e/dolphinscheduler/remote/NettyRemotingClient.java 53.52% <0.00%> (+2.81%) ⬆️
...er/master/dispatch/host/assign/RandomSelector.java 83.33% <0.00%> (+5.55%) ⬆️

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 3258438...aa7cc0c. Read the comment docs.

@sonarcloud
Copy link

sonarcloud bot commented Jun 10, 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

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM

@SbloodyS SbloodyS added bug Something isn't working backend labels Jun 10, 2022
@SbloodyS SbloodyS added this to the 3.0.0-beta-2 milestone Jun 10, 2022
@zhongjiajie
Copy link
Member

zhongjiajie commented Jun 10, 2022

If we set database to mysql in dolphinscheduler_env.sh, when running upgrade-schema.sh in tools, will get errors like Caused by: java.lang.RuntimeException: Driver com.mysql.cj.jdbc.Driver claims to not accept jdbcUrl, jdbc:postgresql://127.0.0.1:5432/dolphinscheduler

Does it means we should also set SPRING_DATASOURCE_URL in dolphinscheduler_env.sh. you adding in this PR only a default value of database connection, if user database not in the same mechine of dolphinscheduler service it will throw error too.

@zhongjiajie zhongjiajie added the discussion discussion label Jun 10, 2022
@zhongjiajie
Copy link
Member

zhongjiajie commented Jun 13, 2022

@EricGao888 could you try it again after #10414 merged

@EricGao888
Copy link
Member Author

@EricGao888 could you try it again after #10414 merged

Sure, will check it as soon as #10414 merged

@EricGao888
Copy link
Member Author

@EricGao888 could you try it again after #10414 merged

@zhongjiajie @SbloodyS It works fine now. I'm quite curious about how you guys made it work? How did PR #10414 affect upgrade-schema.sh? Very interesting.

@EricGao888
Copy link
Member Author

export DATABASE=${DATABASE:-h2}

This field DATABASE is the key to letting the Spring choose the active profile.

And the active profile will choose the DB driver-class-name automatically.

dolphinscheduler-standalone-server/src/main/bin/start.sh miss this line.

So you seted the MySQL URL and other information, but the DB driver-class-name is still PostgreSQL, So you can see the error is Caused by: java.lang.RuntimeException: Driver com.mysql.cj.jdbc.Driver claims to not accept jdbcUrl, jdbc:postgresql://127.0.0.1:5432/dolphinscheduler

And the more detailed information about why we don't need to define URL in YAML file, you can see #10103 (comment)

Still a little bit confused. After I encountered that ...not accept... error, I stopped standalone-server, then reran bash upgrade-schema.sh in tools/bin and still got that error. Was it caused by cache or anything else?

@qingwli
Copy link
Member

qingwli commented Jun 13, 2022

My understanding has some problems. I'll try locally and talk later

@EricGao888
Copy link
Member Author

My understanding has some problems. I'll try locally and talk later

Sure, thx for the explanation : )

@kezhenxu94
Copy link
Member

If we set database to mysql in dolphinscheduler_env.sh, when running upgrade-schema.sh in tools, will get errors like Caused by: java.lang.RuntimeException: Driver com.mysql.cj.jdbc.Driver claims to not accept jdbcUrl, jdbc:postgresql://127.0.0.1:5432/dolphinscheduler

Does it means we should also set SPRING_DATASOURCE_URL in dolphinscheduler_env.sh. you adding in this PR only a default value of database connection, if user database not in the same mechine of dolphinscheduler service it will throw error too.

We don't need to set that. What we want in the yaml is indeed default value. So before and after this PR when users set up in their production environment they are expected to set SPRING_DATASOURCE_URL. This PR is just for convenience that some users/developers want to test locally and the MYSQL url happens to be localhost.

P.S. the reason why I write export SPRING_DATASOURCE_URL without a value is that, users might just set the variable without exporting it, that might not work in Spring so we just export it with the value users set.

SPRING_DATASOURCE_URL=xxxx # note no "export"
daemon.sh start standalone-server

@EricGao888
Copy link
Member Author

EricGao888 commented Jun 13, 2022

If we set database to mysql in dolphinscheduler_env.sh, when running upgrade-schema.sh in tools, will get errors like Caused by: java.lang.RuntimeException: Driver com.mysql.cj.jdbc.Driver claims to not accept jdbcUrl, jdbc:postgresql://127.0.0.1:5432/dolphinscheduler

Does it means we should also set SPRING_DATASOURCE_URL in dolphinscheduler_env.sh. you adding in this PR only a default value of database connection, if user database not in the same mechine of dolphinscheduler service it will throw error too.

We don't need to set that. What we want in the yaml is indeed default value. So before and after this PR when users set up in their production environment they are expected to set SPRING_DATASOURCE_URL. This PR is just for convenience that some users/developers want to test locally and the MYSQL url happens to be localhost.

P.S. the reason why I write export SPRING_DATASOURCE_URL without a value is that, users might just set the variable without exporting it, that might not work in Spring so we just export it with the value users set.

SPRING_DATASOURCE_URL=xxxx # note no "export"
daemon.sh start standalone-server

This makes sense to me. username and password are easily kept in mind, but jdbc:mysql://127.0.0.1:3306/dolphinscheduler?useUnicode=true&characterEncoding=UTF-8 is hardly possible. Adding this default value in application.yaml will save the trouble copying pasting every time.

@zhongjiajie
Copy link
Member

And did we still need this if only work for someone who run database service in the same host with tools script running?

@kezhenxu94
Copy link
Member

And did we still need this if only work for someone who run database service in the same host with tools script running?

Hi, as I said in #10399 (comment) we can have this default value for the sake of convenience, and can be referenced if users want to configure their own database address (copy this by replacing the IP/host).

@kezhenxu94 kezhenxu94 merged commit 42d4aba into apache:dev Jun 14, 2022
devosend pushed a commit that referenced this pull request Jun 18, 2022
hstdream pushed a commit to hstdream/dolphinscheduler that referenced this pull request Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working discussion discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [Tools] DB configurations not work for tools
6 participants