-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
bugfix: fix data source initialize more times #2670
bugfix: fix data source initialize more times #2670
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2670 +/- ##
=============================================
+ Coverage 50.65% 50.91% +0.25%
- Complexity 2812 2814 +2
=============================================
Files 558 558
Lines 17914 17936 +22
Branches 2088 2126 +38
=============================================
+ Hits 9075 9132 +57
+ Misses 7947 7934 -13
+ Partials 892 870 -22
|
core/src/main/java/io/seata/core/store/db/DataSourceGenerator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/seata/core/store/db/AbstractDataSourceGenerator.java
Outdated
Show resolved
Hide resolved
@l81893521 Whether to need to adjust the connection pool of the default number of connections? |
ok, DEFAULT_DB_MAX_CONN already change to 20 |
|
||
@Override | ||
public DataSource provide() { | ||
return getDataSource(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common method, move to abstract class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
private final String datasourceType = "dbcp"; | ||
|
||
@Test | ||
public void testDbcpDataSourceProvider() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can put the test case in the abstract class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Pay attention to the package order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
*/ | ||
public class AbstractDataSourceProviderTest { | ||
|
||
private final String dbcpDatasourceType = "dbcp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge dataSourceType to array. for each test dataSource not null? simplify test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ⅰ. Describe what this PR did
fix data source initialize more than one time cause the data source instance is not a single instance.
Ⅱ. 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