Skip to content

datasource test and sql task Remove connection pool issues is #14179#14193

Merged
SbloodyS merged 6 commits intoapache:devfrom
xuhui1231:dev
Jul 21, 2023
Merged

datasource test and sql task Remove connection pool issues is #14179#14193
SbloodyS merged 6 commits intoapache:devfrom
xuhui1231:dev

Conversation

@xuhui1231
Copy link
Contributor

@xuhui1231 xuhui1231 commented May 24, 2023 •
edited by zhongjiajie
Loading

close: #14179

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@xuhui1231 xuhui1231 requested a review from caishunfeng as a code owner May 24, 2023 08:32
@zhongjiajie zhongjiajie mentioned this pull request May 24, 2023
Closed
@MonsterChenzhuo
Copy link
Contributor

MonsterChenzhuo commented May 24, 2023 •
edited
Loading

hi @xuhui1231 I am very sorry. I understand the problem #14183 that this PR is currently trying to solve. I think our content has been repeated. After the communication in the weekly meeting, I will discuss and decide to remove the connection pool under daatasource, so I am still working on it Currently this works.

@zhaohehuhu
Copy link
Contributor

zhaohehuhu commented May 25, 2023 •
edited
Loading

hi @xuhui1231 I am very sorry. I understand the problem #14183 that this PR is currently trying to solve. I think our content has been repeated. After the communication in the weekly meeting, I will discuss and decide to remove the connection pool under daatasource, so I am still working on it Currently this works.

haha. I also made a PR that wound like to remove connection pool in Kyuubi datasource #14190

@MonsterChenzhuo
Copy link
Contributor

MonsterChenzhuo commented May 25, 2023 •
edited
Loading

I suggest creating an "issues" or task breakdown to handle the multiple data sources. Everyone can choose the sources they are interested in and make modifications for testing purposes.
What do you all think about this suggestion?

@xuhui1231
Copy link
Contributor Author

xuhui1231 commented May 25, 2023

I suggest creating an "issues" or task breakdown to handle the multiple data sources. Everyone can choose the sources they are interested in and make modifications for testing purposes. What do you all think about this suggestion?

OH，That's a good idea，After I test I will create Issues

@zhongjiajie
Copy link
Member

zhongjiajie commented May 26, 2023

@xuhui1231 Hi, please run command mvn spotless:apply to format our code and docs please

@zhongjiajie zhongjiajie added the first time contributor First-time contributor label May 26, 2023
@zhongjiajie zhongjiajie mentioned this pull request Jul 20, 2023
Closed
3 tasks
@zhongjiajie zhongjiajie added bug Something isn't working 3.1.x for 3.1.x version labels Jul 20, 2023
@zhongjiajie
Copy link
Member

zhongjiajie commented Jul 20, 2023

https://github.com/apache/dolphinscheduler/actions/runs/5607655761/jobs/10259960705?pr=14193 it seems some UT error currently, do you have time to correct the fail CI

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2023 •
edited
Loading

Codecov Report

Merging #14193 (1d7b2e7) into dev (d92e2b3) will increase coverage by 0.05%.
The diff coverage is 0.00%.

❗ Current head 1d7b2e7 differs from pull request most recent head b7124e4. Consider uploading reports for the commit b7124e4 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14193      +/-   ##
============================================
+ Coverage     38.51%   38.57%   +0.05%     
- Complexity     4573     4577       +4     
============================================
  Files          1260     1260              
  Lines         43804    43748      -56     
  Branches       4834     4824      -10     
============================================
+ Hits          16872    16875       +3     
+ Misses        25057    25001      -56     
+ Partials       1875     1872       -3
Impacted Files Coverage Δ
.../datasource/api/client/CommonDataSourceClient.java 1.92% <0.00%> (-0.46%) ⬇️
.../datasource/azuresql/AzureSQLDataSourceClient.java 14.28% <ø> (+10.28%) ⬆️
...r/plugin/datasource/hive/HiveDataSourceClient.java 4.00% <ø> (+1.50%) ⬆️
...ugin/datasource/kyuubi/KyuubiDataSourceClient.java 10.00% <ø> (+5.23%) ⬆️
.../datasource/redshift/RedshiftDataSourceClient.java 0.00% <ø> (ø)

... and 4 files with indirect coverage changes

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

@xuhui1231
Copy link
Contributor Author

xuhui1231 commented Jul 20, 2023

https://github.com/apache/dolphinscheduler/actions/runs/5607655761/jobs/10259960705?pr=14193 it seems some UT error currently, do you have time to correct the fail CI
I modified ut and submitted the code Just now

zhongjiajie
zhongjiajie approved these changes Jul 20, 2023
View reviewed changes
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, thanks

@zhongjiajie zhongjiajie removed the 3.1.x for 3.1.x version label Jul 21, 2023
@zhongjiajie zhongjiajie added this to the 3.1.8 milestone Jul 21, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 21, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@SbloodyS SbloodyS merged commit e4fb5b3 into apache:dev Jul 21, 2023
@zhongjiajie
Copy link
Member

zhongjiajie commented Jul 21, 2023

thanks @SbloodyS , I will cherry pick to 3.1.8 prepare

@ruanwenjun
Copy link
Member

ruanwenjun commented Jul 24, 2023

This PR will introduce new bug, the datasource client will only keep one connection.

ruanwenjun added a commit that referenced this pull request Jul 24, 2023
@ruanwenjun
Revert "datasource test and sql task Remove connection pool issues is #…
3e9165d 
…14179 (#14193)"

This reverts commit e4fb5b3.
@ruanwenjun ruanwenjun mentioned this pull request Jul 24, 2023
Merged
ruanwenjun added a commit that referenced this pull request Jul 24, 2023
@ruanwenjun
Revert "datasource test and sql task Remove connection pool issues is #…
5f8fdf3 
…14179 (#14193)"

This reverts commit e4fb5b3.
ruanwenjun added a commit that referenced this pull request Jul 24, 2023
@ruanwenjun
Revert "datasource test and sql task Remove connection pool issues is #…
6617e3f 
…14179 (#14193)" (#14626)

This reverts commit e4fb5b3.
@zhongjiajie zhongjiajie mentioned this pull request Jul 25, 2023
Merged
@zhuangchong zhuangchong removed this from the 3.1.8 milestone Jul 28, 2023
zhongjiajie added a commit that referenced this pull request Aug 30, 2023
@xuhui1231 @zhongjiajie
datasource test and sql task Remove connection pool issues is #14179 (#…
2485775 
…14193)

* datasource test and sql task Remove connection pool issues is #14179

* datasource test and sql task Remove connection pool issues is #14179 uniform style

* datasource test and sql task Remove connection pool issues is #14179 uniform style by 20230720

* datasource test and sql task Remove connection pool issues is #14179 uniform style by 20230720

---------

Co-authored-by: xuhui <xuhui@3h1china.com>
Co-authored-by: Jay Chung <zhongjiajie955@gmail.com>
(cherry picked from commit e4fb5b3)
zhongjiajie pushed a commit that referenced this pull request Aug 30, 2023
@ruanwenjun @zhongjiajie
Revert "datasource test and sql task Remove connection pool issues is #…
e13466b 
…14179 (#14193)" (#14626)

This reverts commit e4fb5b3.

(cherry picked from commit 6617e3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhongjiajie zhongjiajie zhongjiajie approved these changes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng

Assignees

No one assigned

Labels

backend bug Something isn't working first time contributor First-time contributor priority:high ready-to-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Driver does not support get/set network timeout for connections. (Method not supported)

8 participants

@xuhui1231 @MonsterChenzhuo @zhaohehuhu @zhongjiajie @codecov-commenter @ruanwenjun @SbloodyS @zhuangchong