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

[Feature][plugin] Support Kyuubi datasource #13642

Merged
merged 34 commits into from
Apr 18, 2023

Conversation

zhaohehuhu
Copy link
Contributor

@zhaohehuhu zhaohehuhu commented Feb 28, 2023

Purpose of the pull request

close #12535
This PR is to Introduce Kyuubi as datasource, so users can use Kyuubi as SQL proxy instead of Hive datasource, which can get more feature support powered by Kyuubi.

Brief change log

Verify this pull request

This pull request is code cleanup with some 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

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.

Please check the failed CI. Thanks.

Copy link
Member

@fuchanghai fuchanghai left a comment

Choose a reason for hiding this comment

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

where did you use this parameter datawarehouse ?

@zhaohehuhu
Copy link
Contributor Author

where did you use this parameter datawarehouse ?

will check later

@EricGao888
Copy link
Member

@zhaohehuhu CI failed by style check. Could u plz run mvn spotless:apply? Spotless will automatically fix those errors.

import org.apache.dolphinscheduler.common.utils.PropertyUtils;
import org.apache.dolphinscheduler.plugin.datasource.api.client.CommonDataSourceClient;
import org.apache.dolphinscheduler.plugin.datasource.api.provider.JDBCDataSourceProvider;
import org.apache.dolphinscheduler.plugin.datasource.kyuubi.security.*;
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid using wildcard imports and fix it before running spotless cmd, or spotless will directly remove it and lead to compilation failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@zhaohehuhu
Copy link
Contributor Author

@zhaohehuhu CI failed by style check. Could u plz run mvn spotless:apply? Spotless will automatically fix those errors.

OK

@zhaohehuhu
Copy link
Contributor Author

where did you use this parameter datawarehouse ?

fixed

@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2023

Codecov Report

Merging #13642 (1ab382c) into dev (5f94c8c) will increase coverage by 0.04%.
The diff coverage is 58.82%.

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

@@             Coverage Diff              @@
##                dev   #13642      +/-   ##
============================================
+ Coverage     38.87%   38.91%   +0.04%     
- Complexity     4455     4476      +21     
============================================
  Files          1158     1164       +6     
  Lines         42429    42513      +84     
  Branches       4776     4780       +4     
============================================
+ Hits          16496    16546      +50     
- Misses        24118    24151      +33     
- Partials       1815     1816       +1     
Impacted Files Coverage Δ
...cheduler/common/constants/DataSourceConstants.java 0.00% <ø> (ø)
.../org/apache/dolphinscheduler/spi/enums/DbType.java 0.00% <0.00%> (ø)
...ugin/datasource/kyuubi/KyuubiDataSourceClient.java 4.76% <4.76%> (ø)
...gin/datasource/kyuubi/KyuubiDataSourceChannel.java 50.00% <50.00%> (ø)
...source/kyuubi/param/KyuubiDataSourceProcessor.java 76.92% <76.92%> (ø)
...asource/kyuubi/KyuubiDataSourceChannelFactory.java 100.00% <100.00%> (ø)
...datasource/kyuubi/param/KyuubiConnectionParam.java 100.00% <100.00%> (ø)
...asource/kyuubi/param/KyuubiDataSourceParamDTO.java 100.00% <100.00%> (ø)

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

}
hosts.deleteCharAt(hosts.length() - 1);
kyuubiDataSourceParamDTO.setHost(hosts.toString());
kyuubiDataSourceParamDTO.setPort(Integer.parseInt(hostPortArray[0].split(Constants.COLON)[1]));

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException

Potential uncaught 'java.lang.NumberFormatException'.
@zhaohehuhu
Copy link
Contributor Author

Hi All, what should I do next ? @EricGao888 @fuchanghai

@fuchanghai
Copy link
Member

Hi All, what should I do next ? @EricGao888 @fuchanghai
image

can you add related license?

@fuchanghai
Copy link
Member

@zhaohehuhu You can refer to this #12379
企业微信截图_16780899064445

@zhaohehuhu
Copy link
Contributor Author

Hi All, what should I do next ? @EricGao888 @fuchanghai
image

can you add related license?

Sure.

@@ -43,7 +43,8 @@ public enum DbType {
TRINO(12, "trino"),
STARROCKS(13, "starrocks"),
AZURESQL(14, "azuresql"),
DAMENG(15, "dameng");
DAMENG(15, "dameng"),
KYUUBI(18, "kyuubi");
Copy link
Member

Choose a reason for hiding this comment

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

why not set 16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not set 16?

cause 16 is occupied by other datasources implemented by ourselves

Copy link
Member

Choose a reason for hiding this comment

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

how about use 17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

17 is used by ssh now

@zhaohehuhu
Copy link
Contributor Author

@zhaohehuhu You can refer to this #12379 企业微信截图_16780899064445

should be fixed. Please hep check it @fuchanghai

@fuchanghai
Copy link
Member

fuchanghai commented Mar 7, 2023

@zhaohehuhu You can refer to this #12379 企业微信截图_16780899064445

should be fixed. Please hep check it @fuchanghai
maybe you need to add kyuubi in this file
image
image

pan3793
pan3793 previously approved these changes Apr 10, 2023
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

Thanks @zhaohehuhu, LGTM on my side

@zhaohehuhu
Copy link
Contributor Author

Thanks @zhaohehuhu, LGTM on my side

Thanks. Buddy.

@zhaohehuhu
Copy link
Contributor Author

@SbloodyS plz help run CI

@SbloodyS
Copy link
Member

@SbloodyS plz help run CI

Done.

@zhongjiajie
Copy link
Member

approval to run CI

@zhaohehuhu
Copy link
Contributor Author

approval to run CI
plz help run

@zhaohehuhu zhaohehuhu closed this Apr 11, 2023
@zhaohehuhu zhaohehuhu reopened this Apr 11, 2023
@zhaohehuhu zhaohehuhu closed this Apr 13, 2023
@zhaohehuhu zhaohehuhu reopened this Apr 13, 2023
@zhaohehuhu
Copy link
Contributor Author

Hi All, what should I do next ? @EricGao888 @fuchanghai @Radeity

@sonarcloud
Copy link

sonarcloud bot commented Apr 13, 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 28 Code Smells

64.2% 64.2% Coverage
0.0% 0.0% Duplication

@zhaohehuhu
Copy link
Contributor Author

Hi @EricGao888 @fuchanghai @Radeity, it seems there is no error anymore for this PR.

Copy link
Member

@fuchanghai fuchanghai left a comment

Choose a reason for hiding this comment

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

LGTM on my side

@zhongjiajie zhongjiajie merged commit c0126b7 into apache:dev Apr 18, 2023
1 check passed
@zhaohehuhu
Copy link
Contributor Author

Thanks all

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 document feature new feature plug-in UI ui and front end related
Projects
None yet
10 participants