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

refactor: Restructure SQLRecognizer and UndoExecutor #1883

Merged
merged 15 commits into from
Nov 13, 2019

Conversation

booogu
Copy link
Contributor

@booogu booogu commented Nov 9, 2019

<! -- make sure you have read and understood the contribution guide -- >

###I. describe what this PR has done
On the side of RM,

  1. Add UndoExecutorGroupand and its corresponding factory. The UndoExecutorGroup interface abstracts executor that all kinds of databases need to provide (currently implemented by MySQL and Oracle)

  2. Add SqlRecognizerGroup and its corresponding factory. SqlRecognizerGroup abstracts the recognizer (currently mqsql and Oracle) that all kinds of databases need to provide,

The benefits of this reconstruction are as follows:

  1. Eliminate redundant code.

  2. Enhance the scalability of the project (a new type of database can be added on RM side through resource configuration file)

  3. When a new database type is to be added in the future, the capabilities it needs to provide will be roughly divided into three dimensions:

(1) extend AbstractUndoLogManager. (An existing abstraction provided in a previous PR,)

(2) implement UndoExecutorGroup.

(3) implement SQLRecognizerGroup.

To sum up, after refactoring, For the provider (the adaptation of each database type), will be defined and abstracted more clearly and cleanly; For the user (where to get the instance of executor or recognizer), it will be more convenient by calling factory methods directly.

###Does this request solve a problem?

<! -- if so, add "fixesාxxx" to the next line, for example, fixes񖓿97.

###III. why not add test cases (unit test / integration test)?

This PR is only for the adjustment of code structure, not involving the change of specific logic.

###IV. describe how to verify

It only adjusts the internal abstraction level and calling mode, without any specific logical changes. After pulling the latest code, test the official Seata demo project, and conduct overall verification.

###V. special notes for review

Note that in this submission, the two factories I provided (undoexecutor groupfactory and sqlrecognizergroupfactory) are the instances created directly by using the EnhancedServiceLoader to load specific implementation classes from the resource file,

This is mainly to consider: Although there are only two kinds of database adaptations now: mqsql and Oracle, in the future, when other developer expand other types of databases, they can directly add resource file to provide their implemention without changing the existing code.

@codecov-io
Copy link

codecov-io commented Nov 9, 2019

Codecov Report

Merging #1883 into develop will increase coverage by <.01%.
The diff coverage is 82.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #1883      +/-   ##
============================================
+ Coverage       55.3%   55.3%   +<.01%     
- Complexity      2388    2408      +20     
============================================
  Files            422     428       +6     
  Lines          14300   14318      +18     
  Branches        1702    1659      -43     
============================================
+ Hits            7908    7918      +10     
+ Misses          5678    5672       -6     
- Partials         714     728      +14
Impacted Files Coverage Δ Complexity Δ
...source/sql/druid/MySqlOperateRecognizerHolder.java 100% <100%> (ø) 6 <6> (?)
...datasource/undo/mysql/MySQLUndoExecutorHolder.java 100% <100%> (ø) 5 <5> (?)
...ql/druid/oracle/OracleOperateRecognizerHolder.java 100% <100%> (ø) 6 <6> (?)
...tasource/undo/oracle/OracleUndoExecutorHolder.java 50% <50%> (ø) 2 <2> (?)
.../seata/rm/datasource/undo/UndoExecutorFactory.java 64.28% <69.23%> (+42.85%) 3 <3> (ø) ⬇️
.../rm/datasource/undo/UndoExecutorHolderFactory.java 73.33% <73.33%> (ø) 3 <3> (?)
...asource/sql/SQLOperateRecognizerHolderFactory.java 86.66% <86.66%> (ø) 4 <4> (?)
.../io/seata/rm/datasource/sql/SQLVisitorFactory.java 83.33% <90.9%> (-2.39%) 6 <0> (-6)
...obuf/convertor/BranchRegisterRequestConvertor.java 88% <0%> (-8%) 3% <0%> (ø)
...buf/convertor/GlobalLockQueryRequestConvertor.java 92.59% <0%> (-7.41%) 3% <0%> (ø)
... and 32 more

@zjinlei
Copy link
Contributor

zjinlei commented Nov 9, 2019

Thanks for submitting again.
I am sorry to have closed Https://github.com/seata/seata/pull/1523

@zjinlei zjinlei added the first-time contributor first-time contributor label Nov 9, 2019
@booogu
Copy link
Contributor Author

booogu commented Nov 9, 2019

Thanks for submitting again.
I am sorry to have closed Https://github.com/seata/seata/pull/1523

It doesn't matter. That time, I accidentally messed up the code format locally, making the difference not easy to compare. Moreover, my change was later included in the PR of other developers.

@zjinlei
Copy link
Contributor

zjinlei commented Nov 10, 2019

@ggndnn please review together

@zjinlei zjinlei changed the title Optimize RM side code structure:Restructure SQLRecognizer and UndoExecutor refactor: Restructure SQLRecognizer and UndoExecutor Nov 10, 2019
@zjinlei
Copy link
Contributor

zjinlei commented Nov 11, 2019

@CharmingRabbit Please link your git account email when submitting
refer: https://blog.csdn.net/idealcitier/article/details/82697268

@booogu
Copy link
Contributor Author

booogu commented Nov 11, 2019

@CharmingRabbit Please link your git account email when submitting
refer: https://blog.csdn.net/idealcitier/article/details/82697268

Thanks for the link! I've set it up

@zjinlei
Copy link
Contributor

zjinlei commented Nov 11, 2019

@slievrly
Copy link
Member

@CharmingRabbit please move all @date element, later we will also remove the existing ones, here you are datetime instead of date. @dateis not a standard javadoc element.

@booogu
Copy link
Contributor Author

booogu commented Nov 11, 2019

please resolve CI error
https://travis-ci.org/seata/seata/jobs/610220930?utm_medium=notification&utm_source=github_status
image

This Ci error is caused by the rename of the class. I will solve it later

@booogu
Copy link
Contributor Author

booogu commented Nov 12, 2019

@CharmingRabbit please move all @date element, later we will also remove the existing ones, here you are datetime instead of date. @dateis not a standard javadoc element.

removed

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.

LGTM

@booogu booogu requested a review from zjinlei November 13, 2019 11:25
@zjinlei
Copy link
Contributor

zjinlei commented Nov 13, 2019

retry ci

@zjinlei zjinlei closed this Nov 13, 2019
@zjinlei zjinlei reopened this Nov 13, 2019
@booogu
Copy link
Contributor Author

booogu commented Nov 13, 2019

retry ci

passed

Copy link
Contributor

@zjinlei zjinlei left a comment

Choose a reason for hiding this comment

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

LGTM

@zjinlei zjinlei merged commit 3afc53f into apache:develop Nov 13, 2019
@zjinlei
Copy link
Contributor

zjinlei commented Nov 13, 2019

@CharmingRabbit Thank you for your contribution.
There are 2 minor problems, and the latter will be rectified.
1.NPE: !dbType.equals(JdbcConstants.MYSQL) && !dbType.equals(JdbcConstants.ORACLE)
2.code format

@ggndnn
Copy link
Contributor

ggndnn commented Nov 14, 2019

@ggndnn please review together

Sorry, didn't notice this comment in time.

@booogu
Copy link
Contributor Author

booogu commented Nov 14, 2019

@CharmingRabbit Thank you for your contribution.
There are 2 minor problems, and the latter will be rectified.
1.NPE: !dbType.equals(JdbcConstants.MYSQL) && !dbType.equals(JdbcConstants.ORACLE)
2.code format
OK,got it.
finally,@zjinlei @slievrly thank you very much for your help in my first code contribution.

@slievrly slievrly added this to the 1.0 milestone Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants