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: isolate druid used in rm-datasource to solve druid compatibility issue #1693

Merged
merged 4 commits into from
Feb 14, 2020

Conversation

ggndnn
Copy link
Contributor

@ggndnn ggndnn commented Sep 22, 2019

Ⅰ. Describe what this PR did

This PR was created to to solve druid compatibility issue, because druid was only used to do sql parser, so all druid related code were isolated from users' druid by a separate class loader. All imports from druid in rm-datasource were removed except unit test code. In addition, sql parser part was extracted to several new projects as SPI, e.g., seata-sqlparser-core and seata-sqlparser-druid.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@ggndnn ggndnn force-pushed the feature_isolate_druid branch 4 times, most recently from 3e8d22d to 6453aea Compare September 22, 2019 12:23
@codecov-io
Copy link

codecov-io commented Sep 22, 2019

Codecov Report

Merging #1693 into develop will decrease coverage by 0.02%.
The diff coverage is 30.43%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1693      +/-   ##
=============================================
- Coverage      50.14%   50.11%   -0.03%     
- Complexity      2664     2665       +1     
=============================================
  Files            517      517              
  Lines          17016    17038      +22     
  Branches        2063     2065       +2     
=============================================
+ Hits            8532     8538       +6     
- Misses          7674     7688      +14     
- Partials         810      812       +2
Impacted Files Coverage Δ Complexity Δ
...operties/registry/RegistryZooKeeperProperties.java 44% <25%> (-8.95%) 5 <0> (ø)
...properties/registry/ConfigZooKeeperProperties.java 42.85% <25%> (-10.99%) 4 <0> (ø)
...very/registry/zk/ZookeeperRegisterServiceImpl.java 60.93% <42.85%> (-1.36%) 25 <0> (+1)

@xingfudeshi
Copy link
Member

Good!

@ggndnn ggndnn force-pushed the feature_isolate_druid branch 2 times, most recently from 8e620af to c6ae681 Compare September 23, 2019 02:19
@slievrly slievrly changed the title feature: isolate druid used in rm-datasource to solve druid compatibility issue [WIP]feature: isolate druid used in rm-datasource to solve druid compatibility issue Sep 23, 2019
@ggndnn
Copy link
Contributor Author

ggndnn commented Sep 26, 2019

Wait #1703 merged.

@ggndnn ggndnn marked this pull request as ready for review January 19, 2020 06:09
@ggndnn ggndnn changed the title [WIP]feature: isolate druid used in rm-datasource to solve druid compatibility issue feature: isolate druid used in rm-datasource to solve druid compatibility issue Jan 19, 2020
@ggndnn ggndnn force-pushed the feature_isolate_druid branch 2 times, most recently from b7cc9b7 to b9993ba Compare January 19, 2020 07:13
@zjinlei zjinlei added this to the 1.1.0 milestone Jan 21, 2020
@ggndnn ggndnn force-pushed the feature_isolate_druid branch 6 times, most recently from 65358c3 to b71f94f Compare February 6, 2020 08:44
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

check the result is ok.

Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

LGTM.

@slievrly slievrly merged commit 4167192 into apache:develop Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants