[Improvement][plugin] decommission connection pool in kyuubi #14190

zhaohehuhu · 2023-05-24T06:14:58Z

Purpose of the pull request

close #14189

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

zhaohehuhu · 2023-05-25T02:02:59Z

@caishunfeng @SbloodyS plz review

...hinscheduler-task-sql/src/main/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTask.java

codecov-commenter · 2023-06-16T08:58:55Z

Codecov Report

Merging #14190 (1ae81f5) into dev (d6a4e99) will increase coverage by 0.01%.
The diff coverage is 0.00%.

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

@@             Coverage Diff              @@
##                dev   #14190      +/-   ##
============================================
+ Coverage     38.52%   38.53%   +0.01%     
- Complexity     4575     4579       +4     
============================================
  Files          1260     1260              
  Lines         43804    43821      +17     
  Branches       4834     4833       -1     
============================================
+ Hits          16874    16885      +11     
- Misses        25057    25063       +6     
  Partials       1873     1873

Impacted Files	Coverage Δ
...ugin/datasource/kyuubi/KyuubiDataSourceClient.java	`2.63% <0.00%> (-2.14%)`	⬇️

... and 4 files with indirect coverage changes

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

zhaohehuhu · 2023-06-20T06:12:15Z

@caishunfeng @SbloodyS plz review

zhaohehuhu · 2023-06-28T09:05:20Z

@caishunfeng @SbloodyS Is there anything that need to improve ? Thanks.

SbloodyS

Please check failed CI.

zhaohehuhu · 2023-07-13T07:21:34Z

Please check failed CI.

looks like the CI failed due to zookeeper connect timeout

SbloodyS · 2023-07-19T06:21:25Z

...hinscheduler-task-sql/src/main/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTask.java

+            if (DbType.valueOf(sqlParameters.getType()) == KYUUBI) {
+                log.info("full jdbc url : {}, user : {} begin to build a connection",
+                        DataSourceUtils.getJdbcUrl(DbType.KYUUBI, baseConnectionParam), baseConnectionParam.getUser());
+                Class.forName(baseConnectionParam.getDriverClassName());
+                connection = DriverManager.getConnection(baseConnectionParam.getJdbcUrl(),
+                        baseConnectionParam.getUser(), baseConnectionParam.getPassword());
+            } else {
+                connection =
+                        DataSourceClientProvider.getInstance().getConnection(DbType.valueOf(sqlParameters.getType()),
+                                baseConnectionParam);
+            }


I'm not sure if this is a good approach. If there are new data sources or task types in the future that need to share the same implementation.

Yup. It's not a good way to do it. Can we extract part of code into getConnection method ?

ruanwenjun · 2023-07-19T10:06:38Z

...c/main/java/org/apache/dolphinscheduler/plugin/datasource/kyuubi/KyuubiDataSourceClient.java

        while (connection == null) {
            try {
-                connection = dataSource.getConnection();
+                connection = driverManagerDataSource.getConnection();


Please add max retry times here, this is dangerous code, in fact, it's better to generate a new connection here, rather than use driverManagerDataSource.getConnection();

ruanwenjun · 2023-07-19T10:08:51Z

...c/main/java/org/apache/dolphinscheduler/plugin/datasource/kyuubi/KyuubiDataSourceClient.java

+        this.driverManagerDataSource =
+                new DriverManagerDataSource(DataSourceUtils.getJdbcUrl(DbType.KYUUBI, baseConnectionParam),
+                        baseConnectionParam.getUser(), PasswordUtils.decodePassword(baseConnectionParam.getPassword()));
+        driverManagerDataSource.setDriverClassName(baseConnectionParam.getDriverClassName());
+        this.jdbcTemplate = new JdbcTemplate(driverManagerDataSource);
+        log.info("Init {} {} success.", getClass().getName(),
+                DataSourceUtils.getJdbcUrl(DbType.KYUUBI, baseConnectionParam));


We don't need to use DriverManagerDataSource here, we can directly generate new connections.

DriverManagerDataSource should create a new connection each time, so we possibly can stick to that.

sonarqubecloud · 2023-07-21T05:54:38Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

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

zhaohehuhu requested review from SbloodyS, caishunfeng and zhuangchong as code owners May 24, 2023 06:14

github-actions bot added the backend label May 24, 2023

zhaohehuhu mentioned this pull request May 25, 2023

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

Merged

SbloodyS reviewed May 25, 2023

View reviewed changes

...hinscheduler-task-sql/src/main/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTask.java Outdated Show resolved Hide resolved

SbloodyS assigned zhaohehuhu May 25, 2023

SbloodyS added improvement make more easy to user or prompt friendly 3.2.0 for 3.2.0 version labels May 25, 2023

SbloodyS added this to the 3.2.0 milestone May 25, 2023

zhaohehuhu force-pushed the Improvement-14189 branch from bf81161 to 4c19f77 Compare July 12, 2023 09:26

SbloodyS reviewed Jul 13, 2023

View reviewed changes

zhaohehuhu force-pushed the Improvement-14189 branch 2 times, most recently from 342e80d to 2355b0b Compare July 19, 2023 02:36

SbloodyS requested review from EricGao888, Radeity and ruanwenjun July 19, 2023 03:23

SbloodyS reviewed Jul 19, 2023

View reviewed changes

ruanwenjun force-pushed the Improvement-14189 branch from 464f405 to fd86a51 Compare July 19, 2023 10:03

ruanwenjun requested changes Jul 19, 2023

View reviewed changes

zhaohehuhu force-pushed the Improvement-14189 branch 2 times, most recently from e35eb2d to b0606be Compare July 20, 2023 05:12

zhaohehuhu added 3 commits July 21, 2023 13:21

[Improvement][plugin] decommission connection pool in kyuubi

18f5bd4

apply spotless

c928bb6

apply spotless

04dcba4

zhaohehuhu added 6 commits July 21, 2023 13:21

fix import

36a5374

fix UT

1ed83b7

Refactor code

ce5c369

Refactor code

89d4c9a

set max retry attempts for building connection

4eacbb2

fix sonar bug

63ab6d9

ruanwenjun force-pushed the Improvement-14189 branch from 5249fe5 to 63ab6d9 Compare July 21, 2023 05:21

get rid of loop

0dd3aba

zhaohehuhu and others added 2 commits July 21, 2023 14:13

refactor code

45a50f9

Merge branch 'dev' into Improvement-14189

b518296

zhaohehuhu closed this Jul 21, 2023

[Improvement][plugin] decommission connection pool in kyuubi #14190

[Improvement][plugin] decommission connection pool in kyuubi #14190

Uh oh!

Conversation

zhaohehuhu commented May 24, 2023

Purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

zhaohehuhu commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zhaohehuhu commented Jun 20, 2023

Uh oh!

zhaohehuhu commented Jun 28, 2023

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

zhaohehuhu commented Jul 13, 2023

Uh oh!

SbloodyS Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

zhaohehuhu Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

zhaohehuhu Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

ruanwenjun Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaohehuhu Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

zhaohehuhu Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

ruanwenjun Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

zhaohehuhu Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhaohehuhu commented May 25, 2023 •

edited

Loading

codecov-commenter commented Jun 16, 2023 •

edited

Loading

ruanwenjun Jul 19, 2023 •

edited

Loading