-
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
refactor: log store sql with spi #2369
refactor: log store sql with spi #2369
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2369 +/- ##
=============================================
+ Coverage 51.36% 51.66% +0.30%
- Complexity 2663 2686 +23
=============================================
Files 529 535 +6
Lines 16983 16932 -51
Branches 2051 2027 -24
=============================================
+ Hits 8723 8748 +25
+ Misses 7432 7372 -60
+ Partials 828 812 -16
|
* @author will | ||
*/ | ||
@LoadLevel(name = "oceanbase") | ||
public class OceanbaseLogStoreSqls extends MysqlLogStoreSqls { |
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.
Why here inherit mysql?,This will lead to a strong correlation between Oceanbase and Mysql.
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.
Because I think oceanbase was compatible with mysql
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.
Because I think oceanbase was compatible with mysql
is ok.
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.
Tidb as same as mysql too, please add.
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.
PTAL.
*/ | ||
public class LogStoreSqlsFactory { | ||
|
||
private static Map<String, LogStoreSqls> LOG_STORE_SQLS_MAP = new ConcurrentHashMap<>(); |
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.
Should not support multiple database, Can be changed to a single.
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.
Please add a 'log' directory under the 'sql' directory.
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.
…_sql_with_spi # Conflicts: # server/src/main/java/io/seata/server/storage/db/store/LogStoreSqls.java # server/src/test/java/io/seata/server/lock/db/sql/log/LogStoreSqlsFactoryTest.java
…o refactor_log_store_sql_with_spi
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
/** | ||
* @author will | ||
*/ | ||
public class LogStoreSqlsFactory { |
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.
#2135 has merged, LogStoreSqlsFactory can be deleted.
|
||
@Override | ||
public String getInsertGlobalTransactionSQL(String globalTable) { | ||
throw new NotSupportYetException("unknown dbType:" + CONFIG.getConfig(ConfigurationKeys.STORE_DB_TYPE)); |
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.
Suggest:
return getInsertGlobalTransactionSQL().replace(GLOBAL_TABLE_PLACEHOLD, globalTable);
abstract String getInsertGlobalTransactionSQL();
Same for other sql
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.
OK
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
…o refactor_log_store_sql_with_spi
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
refactor log store sqls with spi
Ⅱ. 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