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.

Sign up for GitHub

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

Jump to bottom

Data source plugin support xugu database #15601

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from
Open

Data source plugin support xugu database #15601

wants to merge 25 commits into from

Conversation

mrliufox
Copy link

Purpose of the pull request

Data source plugin support xugu database

Brief change log

Data source plugin support xugu database

Verify this pull request

This change added tests and can be verified as follows:

  • Added XuguDataSourceChannelFactoryTest to verify the change.
  • Added XuguDataSourceChannelTest to verify the change.

@github-actions github-actions bot added UI ui and front end related backend document labels Feb 20, 2024
@mrliufox
Copy link
Author

❌ (Please check if there are PRs that already have a ready-to-merge label and can be merged, if exists please merge them first.)

But I have no merging branches

@mrliufox
Copy link
Author

❌ (Please check if there are PRs that already have a ready-to-merge label and can be merged, if exists please merge them first.)

I have no merging branches

EricGao888
EricGao888 reviewed Feb 20, 2024
View reviewed changes
dolphinscheduler-datasource-plugin/dolphinscheduler-datasource-xugu/.gitignore Outdated
@@ -0,0 +1,38 @@
target/
Copy link
Member

Choose a reason for hiding this comment

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

please remove this file

@mrliufox
Copy link
Author

❌ (Please check if there are PRs that already have a ready-to-merge label and can be merged, if exists please merge them first.)

I have no merging branches

songjianet
songjianet previously approved these changes Feb 21, 2024
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024 •
edited

Codecov Report

Attention: Patch coverage is 65.67164% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 39.56%. Comparing base (27d0563) to head (ed7ca12).

❗ Current head ed7ca12 differs from pull request most recent head 2f089f8. Consider uploading reports for the commit 2f089f8 to get more accurate results

Files Patch % Lines
...datasource/xugu/param/XuguDataSourceProcessor.java 72.00% 12 Missing and 2 partials ⚠️
...gin/datasource/xugu/XuguAdHocDataSourceClient.java 0.00% 2 Missing ⚠️
.../plugin/datasource/xugu/XuguDataSourceChannel.java 33.33% 2 Missing ⚠️
...in/datasource/xugu/XuguPooledDataSourceClient.java 0.00% 2 Missing ⚠️
.../org/apache/dolphinscheduler/spi/enums/DbType.java 0.00% 2 Missing ⚠️
.../datasource/xugu/param/XuguDataSourceParamDTO.java 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15601      +/-   ##
============================================
- Coverage     39.59%   39.56%   -0.03%     
+ Complexity     5010     5007       -3     
============================================
  Files          1347     1354       +7     
  Lines         45646    45712      +66     
  Branches       4892     4894       +2     
============================================
+ Hits          18073    18087      +14     
- Misses        25648    25699      +51     
- Partials       1925     1926       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SbloodyS
SbloodyS requested changes Feb 21, 2024
View reviewed changes
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.

Related PR: #14929

@mrliufox
Copy link
Author

_Error: One or more dependencies were identified with vulnerabilities that have a CVSS score greater than or equal to '7.0': _

Please guide me on how to deal with this problem, thank you very much.

@mrliufox
Copy link
Author

what should i do next?

@SbloodyS
Copy link
Member

SbloodyS commented Feb 23, 2024 •
edited

Maybe you did not subscribe dev mail list. You should subscribe it first. @mrliufox

@mrliufox
Copy link
Author

Maybe you did not subscribe dev mail list. You should subscribe it first. @mrliufox

how to subscribe dev mail list? Please guide, thank you very much.

@SbloodyS
Copy link
Member

You can take a look at #8847. @mrliufox

@mrliufox
Copy link
Author

mrliufox commented Feb 23, 2024 •
edited

You can take a look at #8847. @mrliufox

Thank you very much, I have subscribed to the dev email list and sented an email to dev@dolphinscheduler.apache.org to explain the importance of dolphinscheduler for xugu database support

@mrliufox
Copy link
Author

what should i do next?

@caishunfeng caishunfeng added the feature new feature label Feb 28, 2024
@mrliufox
Copy link
Author

mrliufox commented Mar 4, 2024

help please ! what should i do next

@mrliufox
Copy link
Author

what should i do next ?

@SbloodyS
Copy link
Member

Currently, there are no maintainer is willing to accept the maintenance of this data source. So this PR will be temporarily put on hold until a maintainer is willing to continue maintaining it. @mrliufox

fuchanghai
fuchanghai requested changes Mar 20, 2024
View reviewed changes
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.

image You may need to add some logic in the datax plugin

fuchanghai
fuchanghai reviewed Apr 14, 2024
View reviewed changes
dolphinscheduler-spi/src/main/java/org/apache/dolphinscheduler/spi/enums/DbType.java Outdated

K8S(26, "k8s");
K8S(26, "k8s"),
XUGU(27, "xugu"),
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can use XUGU(27, "xugu") instead of XUGU(27, "xugu"),.

fuchanghai
fuchanghai approved these changes Apr 15, 2024
View reviewed changes
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.

basically LGTM cc @SbloodyS @Gallardot @EricGao888 PTAL

@Amy0104 Amy0104 added 3.3.0 and removed 3.3.0 labels Apr 16, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 16, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

zhongjiajie
zhongjiajie approved these changes Apr 16, 2024
View reviewed changes
Copy link
Member

@zhongjiajie zhongjiajie left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, but it seems @SbloodyS have other oppoin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gallardot Gallardot Gallardot left review comments

@EricGao888 EricGao888 EricGao888 left review comments

@SbloodyS SbloodyS SbloodyS requested changes

@zhongjiajie zhongjiajie zhongjiajie approved these changes

@wangxj3 wangxj3 wangxj3 approved these changes

@fuchanghai fuchanghai fuchanghai approved these changes

@songjianet songjianet songjianet left review comments

@Amy0104 Amy0104 Awaiting requested review from Amy0104 Amy0104 is a code owner

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@mrliufox mrliufox

Labels
backend document don't merge feature new feature UI ui and front end related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet