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-13972][dao] set default value for command #14612

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Conversation

eye-gu
Copy link
Contributor

@eye-gu eye-gu commented Jul 20, 2023

Purpose of the pull request

close #13972

Brief change log

  1. upgrade mysql version
  2. make task depend type default

@@ -56,7 +56,7 @@
<hadoop.version>3.2.4</hadoop.version>
<cron-utils.version>9.1.6</cron-utils.version>
<h2.version>2.2.220</h2.version>
<mysql-connector.version>8.0.16</mysql-connector.version>
<mysql-connector.version>8.0.33</mysql-connector.version>
Copy link
Member

Choose a reason for hiding this comment

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

This is a significant change for users. We should disscuss it. @ruanwenjun @zhongjiajie @EricGao888

Copy link
Member

Choose a reason for hiding this comment

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

it seem 8.0.33 is compatible with 8.0.16, But I want to know is why should we upgrade the version of mysql jdbc? I think we can fix this issue without this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The upgrade is to ensure that the fields in the database are null and the objects are also null, rather than enumerating 0

Copy link
Member

@ruanwenjun ruanwenjun Jul 31, 2023

Choose a reason for hiding this comment

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

The enums are parsed by Mybatis-plus, I don't think this should be related to MySQL driver, is there any related doc?

BTW, if there is a reason to upgrade the MySQL driver, it's needed to use a single PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bug is caused by both the MySQL driver and mybatisplus. The driver returned 0 when the database field was null, and mybatisplus did not correctly use wasnull to determine. You can refer to this link specifically: baomidou/mybatis-plus#5266

Copy link
Member

@ruanwenjun ruanwenjun Aug 1, 2023

Choose a reason for hiding this comment

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

This should be a serious bug, MySQL stores null but the driver returns 0.

I find the bug is ResultSetImpl.getObject() will return a non null value. But I havn't test this.
https://dev.mysql.com/doc/relnotes/connector-j/8.0/en/news-8-0-17.html

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #14612 (a73c948) into dev (cb55476) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head a73c948 differs from pull request most recent head 42584a4. Consider uploading reports for the commit 42584a4 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14612      +/-   ##
============================================
- Coverage     38.71%   38.69%   -0.03%     
+ Complexity     4628     4625       -3     
============================================
  Files          1285     1285              
  Lines         43976    43973       -3     
  Branches       4859     4859              
============================================
- Hits          17025    17014      -11     
- Misses        25071    25079       +8     
  Partials       1880     1880              
Files Changed Coverage Δ
...rg/apache/dolphinscheduler/dao/entity/Command.java 0.00% <ø> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dolphinscheduler-bom/pom.xml Outdated Show resolved Hide resolved
@eye-gu eye-gu changed the title [Fix-13972][dao] upgrade mysql version and make task depend type default [Fix-13972][dao] set default value for command Aug 2, 2023
@ruanwenjun ruanwenjun added improvement make more easy to user or prompt friendly and removed don't merge discussion discussion labels Aug 2, 2023
@ruanwenjun ruanwenjun added this to the 3.2.0 milestone Aug 2, 2023
@ruanwenjun ruanwenjun added the 3.2.0 for 3.2.0 version label Aug 2, 2023
@ruanwenjun ruanwenjun force-pushed the Fix-13972 branch 2 times, most recently from 7ced8b7 to 74995f7 Compare August 11, 2023 02:08
@sonarcloud
Copy link

sonarcloud bot commented Aug 12, 2023

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
0.0% 0.0% Duplication

@ruanwenjun ruanwenjun merged commit dde45db into apache:dev Aug 14, 2023
51 of 52 checks passed
biaoma-ty pushed a commit to Kasma-Inc/dolphinscheduler that referenced this pull request Aug 17, 2023
zhongjiajie pushed a commit that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2.0 for 3.2.0 version backend improvement make more easy to user or prompt friendly ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [master] the task depend type is null when recovery suspend
5 participants