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

[FIX-#6505][Dao] upgrade the MySQL driver package for building MySQL jdbcUrl #6708

Merged
merged 10 commits into from
Nov 18, 2021

Conversation

lyggyhmm
Copy link
Contributor

@lyggyhmm lyggyhmm commented Nov 5, 2021

Purpose of the pull request

fix #6505 upgrade the MySQL driver package for building MySQL jdbcUrl

Brief change log

  • replace com.mysql.jdbc.Driver everywhere with com.mysql.cj.jdbc.Driver
  • update related unit tests

…rameter to the static parameter APPEND_PARAMS for building MySQL jdbcUrl

* replace com.mysql.jdbc.Driver everywhere with com.mysql.cj.jdbc.Driver
* append useSSL=false after the static parameter APPEND_PARAMS
@lyggyhmm lyggyhmm changed the title Fix/upgraded mysql driver [DS-6505][Dao] upgrade the MySQL driver package and add the useSSL parameter to the static parameter APPEND_PARAMS for building MySQL jdbcUrl Nov 5, 2021
@zhuangchong zhuangchong added the first time contributor First-time contributor label Nov 6, 2021
lyggyhmm and others added 7 commits November 8, 2021 14:04
# Conflicts:
#	dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/datasource/mysql/MysqlDatasourceProcessor.java
# Conflicts:
#	docker/build/conf/dolphinscheduler/datasource.properties.tpl
#	dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/Constants.java
#	dolphinscheduler-dao/src/main/resources/datasource.properties
#	install.sh
@ruanwenjun ruanwenjun linked an issue Nov 17, 2021 that may be closed by this pull request
3 tasks
zhongjiajie
zhongjiajie previously approved these changes Nov 17, 2021
Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

LGTM now.

@zhongjiajie
Copy link
Member

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

@zhongjiajie
Copy link
Member

zhongjiajie commented Nov 17, 2021

Wait a minute, why should we add file dolphinscheduler-dao/src/main/resources/datasource.properties here? @lyggyhmm
does it add by accident

@lyggyhmm lyggyhmm closed this Nov 17, 2021
@lyggyhmm
Copy link
Contributor Author

Wait a minute, why should we add file dolphinscheduler-dao/src/main/resources/datasource.properties here? @lyggyhmm does it add by accident

It seems that there was this file before. It is not enough to see that it was deleted on the 14th in the submission record. It may be that I had a little accident during fetch.

@lyggyhmm
Copy link
Contributor Author

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

Thank you for your guidance. This is the first time I mention PR here. I may not do well in some places. I will continue to improve in the future.

@zhongjiajie
Copy link
Member

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

Thank you for your guidance. This is the first time I mention PR here. I may not do well in some places. I will continue to improve in the future.

Hi @lyggyhmm you could reopen this PR, and just edited PR describe, we would help you finish your work.
BTW, code look good to me, you just need to remove unnecessary dolphinscheduler-dao/src/main/resources/datasource.properties

@lyggyhmm lyggyhmm reopened this Nov 17, 2021
@lyggyhmm lyggyhmm changed the title [DS-6505][Dao] upgrade the MySQL driver package and add the useSSL parameter to the static parameter APPEND_PARAMS for building MySQL jdbcUrl [FIX-#6505][Dao] upgrade the MySQL driver package for building MySQL jdbcUrl Nov 17, 2021
@lyggyhmm
Copy link
Contributor Author

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

Hi @lyggyhmm , to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe. I would not only connect issue to PR but also close issue automatically when PR is be closed.

Thank you for your guidance. This is the first time I mention PR here. I may not do well in some places. I will continue to improve in the future.

Hi @lyggyhmm you could reopen this PR, and just edited PR describe, we would help you finish your work. BTW, code look good to me, you just need to remove unnecessary dolphinscheduler-dao/src/main/resources/datasource.properties

Now, I modified the PR according to your prompts and re-edited the PR description. Please review

Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

LGTM, @ruanwenjun could you please take a look? If all thing done maybe it could merge to dev now

@codecov-commenter
Copy link

Codecov Report

Merging #6708 (da5e403) into dev (94352a4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #6708      +/-   ##
============================================
+ Coverage     33.07%   33.08%   +0.01%     
- Complexity     1619     1620       +1     
============================================
  Files           433      433              
  Lines         14292    14292              
  Branches       1427     1427              
============================================
+ Hits           4727     4729       +2     
+ Misses         9116     9111       -5     
- Partials        449      452       +3     
Impacted Files Coverage Δ
...pache/dolphinscheduler/spi/task/TaskConstants.java 0.00% <ø> (ø)
...g/apache/dolphinscheduler/spi/utils/Constants.java 0.00% <ø> (ø)
...api/datasource/mysql/MysqlDatasourceProcessor.java 62.85% <100.00%> (ø)
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 53.52% <0.00%> (+1.40%) ⬆️
...org/apache/dolphinscheduler/remote/utils/Host.java 40.00% <0.00%> (+2.22%) ⬆️

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 94352a4...da5e403. Read the comment docs.

@sonarcloud
Copy link

sonarcloud bot commented Nov 17, 2021

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

60.0% 60.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM, If no other concern, I will merge this pr tomorrow.

@ruanwenjun ruanwenjun merged commit 861aaaf into apache:dev Nov 18, 2021
@ruanwenjun
Copy link
Member

@lyggyhmm Pr merged, thanks for your contribution, this is your first contribution, you can contact to @zhongjiajie or me by email, we will help you join developer discussion groups.

@zhongjiajie
Copy link
Member

Hi @lyggyhmm , thanks for your contribution and welcome to join community 🎉 . If you want contribute but could not find issue, maybe you could start in #5689 or just search our issue list https://github.com/apache/dolphinscheduler/issues. Looking forward to your next contribution

@lyggyhmm
Copy link
Contributor Author

@lyggyhmm Pr merged, thanks for your contribution, this is your first contribution, you can contact to @zhongjiajie or me by email, we will help you join developer discussion groups.

Thank you for your affirmation! In my spare time, I will continue to pay attention to the dynamics of the DS community and continue to contribute my own strength! I have been in the DS seed incubator group before, and would like to ask if I can join the contributor group now

@lyggyhmm
Copy link
Contributor Author

Hi @lyggyhmm , thanks for your contribution and welcome to join community 🎉 . If you want contribute but could not find issue, maybe you could start in #5689 or just search our issue list https://github.com/apache/dolphinscheduler/issues. Looking forward to your next contribution

Thank you for your affirmation! It’s an honor to join the community, and I still need your advice in the future

@zhongjiajie
Copy link
Member

Thank you for your affirmation! In my spare time, I will continue to pay attention to the dynamics of the DS community and continue to contribute my own strength! I have been in the DS seed incubator group before, and would like to ask if I can join the contributor group now

@lyggyhmm Will connect you in seed incubator wechat group

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

Successfully merging this pull request may close these issues.

[Feature][Dao] Upgrade com.mysql.jdbc.Driver
5 participants