You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/07/11 14:33:25 UTC
[GitHub] [shardingsphere] yx9o opened a new pull request, #19037: Refactor ImportDatabaseConfigurationHandler.
yx9o opened a new pull request, #19037:
URL: https://github.com/apache/shardingsphere/pull/19037
Fixes #18746 .
Changes proposed in this pull request:
- Refactor ImportDatabaseConfigurationHandler to only allow imports when database is empty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] RaigorJiang commented on a diff in pull request #19037: Refactor ImportDatabaseConfigurationHandler.
Posted by GitBox <gi...@apache.org>.
RaigorJiang commented on code in PR #19037:
URL: https://github.com/apache/shardingsphere/pull/19037#discussion_r918566284
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/distsql/ral/common/updatable/ImportDatabaseConfigurationHandler.java:
##########
@@ -94,73 +93,84 @@ protected void update(final ContextManager contextManager) throws DistSQLExcepti
String databaseName = yamlConfig.getDatabaseName();
checkDatabaseName(databaseName, file);
checkDataSource(yamlConfig.getDataSources(), file);
- updateResources(databaseName, yamlConfig.getDataSources());
- updateRules(databaseName, yamlConfig.getRules());
+ addDatabase(databaseName);
+ addResources(databaseName, yamlConfig.getDataSources());
+ try {
+ addRules(databaseName, yamlConfig.getRules());
+ } catch (DistSQLException ex) {
+ dropDatabase(databaseName);
+ throw ex;
+ }
}
private void checkDatabaseName(final String databaseName, final File file) {
Preconditions.checkNotNull(databaseName, String.format("Property `databaseName` in file `%s` is required.", file.getName()));
- if (!ProxyContext.getInstance().getAllDatabaseNames().contains(databaseName)) {
- throw new DatabaseNotExistedException(databaseName);
+ if (ProxyContext.getInstance().getAllDatabaseNames().contains(databaseName)) {
+ Preconditions.checkState(ProxyContext.getInstance().getDatabase(databaseName).getResource().getDataSources().isEmpty(), "Database `%s` exists.", databaseName);
Review Comment:
The dataSources are checked here, but the message is 'Database `%s` exists', which is not easy to understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] yx9o commented on a diff in pull request #19037: Refactor ImportDatabaseConfigurationHandler.
Posted by GitBox <gi...@apache.org>.
yx9o commented on code in PR #19037:
URL: https://github.com/apache/shardingsphere/pull/19037#discussion_r918533690
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/distsql/ral/common/updatable/ImportDatabaseConfigurationHandler.java:
##########
@@ -94,73 +91,84 @@ protected void update(final ContextManager contextManager) throws DistSQLExcepti
String databaseName = yamlConfig.getDatabaseName();
checkDatabaseName(databaseName, file);
checkDataSource(yamlConfig.getDataSources(), file);
- updateResources(databaseName, yamlConfig.getDataSources());
- updateRules(databaseName, yamlConfig.getRules());
+ addDatabase(databaseName);
+ addResources(databaseName, yamlConfig.getDataSources());
+ try {
+ addRules(databaseName, yamlConfig.getRules());
+ } catch (DistSQLException ex) {
+ dropDatabase(databaseName);
+ throw new RuntimeException(ex.getMessage());
Review Comment:
I made adjustments, please review, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] codecov-commenter commented on pull request #19037: Refactor ImportDatabaseConfigurationHandler.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #19037:
URL: https://github.com/apache/shardingsphere/pull/19037#issuecomment-1181315855
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/19037?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19037](https://codecov.io/gh/apache/shardingsphere/pull/19037?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (65e1127) into [master](https://codecov.io/gh/apache/shardingsphere/commit/8e3604c3bf36346ce6bd6d9b85a4b2ca3eff7433?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e3604c) will **decrease** coverage by `0.07%`.
> The diff coverage is `3.22%`.
```diff
@@ Coverage Diff @@
## master #19037 +/- ##
============================================
- Coverage 59.43% 59.35% -0.08%
+ Complexity 2319 2318 -1
============================================
Files 3803 3797 -6
Lines 54691 54643 -48
Branches 9234 9225 -9
============================================
- Hits 32507 32436 -71
- Misses 19459 19489 +30
+ Partials 2725 2718 -7
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/19037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...hecker/ShardingRuleConfigurationImportChecker.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L2Rpc3RzcWwvcmFsL2NvbW1vbi9jaGVja2VyL1NoYXJkaW5nUnVsZUNvbmZpZ3VyYXRpb25JbXBvcnRDaGVja2VyLmphdmE=) | `10.63% <0.00%> (ø)` | |
| [.../updatable/ImportDatabaseConfigurationHandler.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L2Rpc3RzcWwvcmFsL2NvbW1vbi91cGRhdGFibGUvSW1wb3J0RGF0YWJhc2VDb25maWd1cmF0aW9uSGFuZGxlci5qYXZh) | `17.56% <3.33%> (-58.91%)` | :arrow_down: |
| [...er/standalone/StandaloneContextManagerBuilder.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtbW9kZS9zaGFyZGluZ3NwaGVyZS1tb2RlLXR5cGUvc2hhcmRpbmdzcGhlcmUtc3RhbmRhbG9uZS1tb2RlL3NoYXJkaW5nc3BoZXJlLXN0YW5kYWxvbmUtbW9kZS1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9tb2RlL21hbmFnZXIvc3RhbmRhbG9uZS9TdGFuZGFsb25lQ29udGV4dE1hbmFnZXJCdWlsZGVyLmphdmE=) | `84.61% <0.00%> (-3.39%)` | :arrow_down: |
| [...sphere/dbdiscovery/rule/DatabaseDiscoveryRule.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZGItZGlzY292ZXJ5L3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9kYmRpc2NvdmVyeS9ydWxlL0RhdGFiYXNlRGlzY292ZXJ5UnVsZS5qYXZh) | `66.25% <0.00%> (-2.89%)` | :arrow_down: |
| [...gsphere/spi/type/required/RequiredSPIRegistry.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9zcGkvdHlwZS9yZXF1aXJlZC9SZXF1aXJlZFNQSVJlZ2lzdHJ5LmphdmE=) | `57.14% <0.00%> (-2.86%)` | :arrow_down: |
| [...re/mode/repository/standalone/h2/H2Repository.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtbW9kZS9zaGFyZGluZ3NwaGVyZS1tb2RlLXR5cGUvc2hhcmRpbmdzcGhlcmUtc3RhbmRhbG9uZS1tb2RlL3NoYXJkaW5nc3BoZXJlLXN0YW5kYWxvbmUtbW9kZS1yZXBvc2l0b3J5L3NoYXJkaW5nc3BoZXJlLXN0YW5kYWxvbmUtbW9kZS1yZXBvc2l0b3J5LXByb3ZpZGVyL3NoYXJkaW5nc3BoZXJlLXN0YW5kYWxvbmUtbW9kZS1yZXBvc2l0b3J5LWgyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9tb2RlL3JlcG9zaXRvcnkvc3RhbmRhbG9uZS9oMi9IMlJlcG9zaXRvcnkuamF2YQ==) | `73.17% <0.00%> (-0.91%)` | :arrow_down: |
| [...RuleAlgorithmProviderConfigurationYamlSwapper.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc2hhcmRpbmcveWFtbC9zd2FwcGVyL1NoYXJkaW5nUnVsZUFsZ29yaXRobVByb3ZpZGVyQ29uZmlndXJhdGlvbllhbWxTd2FwcGVyLmphdmE=) | `10.00% <0.00%> (-0.35%)` | :arrow_down: |
| [...ule/EncryptColumnRuleConfigurationYamlSwapper.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC9zaGFyZGluZ3NwaGVyZS1lbmNyeXB0LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL2VuY3J5cHQveWFtbC9zd2FwcGVyL3J1bGUvRW5jcnlwdENvbHVtblJ1bGVDb25maWd1cmF0aW9uWWFtbFN3YXBwZXIuamF2YQ==) | `100.00% <0.00%> (ø)` | |
| [...eadwritesplitting/rule/ReadwriteSplittingRule.java](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtcmVhZHdyaXRlLXNwbGl0dGluZy9zaGFyZGluZ3NwaGVyZS1yZWFkd3JpdGUtc3BsaXR0aW5nLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3JlYWR3cml0ZXNwbGl0dGluZy9ydWxlL1JlYWR3cml0ZVNwbGl0dGluZ1J1bGUuamF2YQ==) | `56.45% <0.00%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/shardingsphere/pull/19037/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/19037?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/19037?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8e3604c...65e1127](https://codecov.io/gh/apache/shardingsphere/pull/19037?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] yx9o commented on a diff in pull request #19037: Refactor ImportDatabaseConfigurationHandler.
Posted by GitBox <gi...@apache.org>.
yx9o commented on code in PR #19037:
URL: https://github.com/apache/shardingsphere/pull/19037#discussion_r918587139
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/distsql/ral/common/updatable/ImportDatabaseConfigurationHandler.java:
##########
@@ -94,73 +93,84 @@ protected void update(final ContextManager contextManager) throws DistSQLExcepti
String databaseName = yamlConfig.getDatabaseName();
checkDatabaseName(databaseName, file);
checkDataSource(yamlConfig.getDataSources(), file);
- updateResources(databaseName, yamlConfig.getDataSources());
- updateRules(databaseName, yamlConfig.getRules());
+ addDatabase(databaseName);
+ addResources(databaseName, yamlConfig.getDataSources());
+ try {
+ addRules(databaseName, yamlConfig.getRules());
+ } catch (DistSQLException ex) {
+ dropDatabase(databaseName);
+ throw ex;
+ }
}
private void checkDatabaseName(final String databaseName, final File file) {
Preconditions.checkNotNull(databaseName, String.format("Property `databaseName` in file `%s` is required.", file.getName()));
- if (!ProxyContext.getInstance().getAllDatabaseNames().contains(databaseName)) {
- throw new DatabaseNotExistedException(databaseName);
+ if (ProxyContext.getInstance().getAllDatabaseNames().contains(databaseName)) {
+ Preconditions.checkState(ProxyContext.getInstance().getDatabase(databaseName).getResource().getDataSources().isEmpty(), "Database `%s` exists.", databaseName);
Review Comment:
Hi, I made adjustments, please review, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] RaigorJiang commented on a diff in pull request #19037: Refactor ImportDatabaseConfigurationHandler.
Posted by GitBox <gi...@apache.org>.
RaigorJiang commented on code in PR #19037:
URL: https://github.com/apache/shardingsphere/pull/19037#discussion_r918491438
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/distsql/ral/common/updatable/ImportDatabaseConfigurationHandler.java:
##########
@@ -94,73 +91,84 @@ protected void update(final ContextManager contextManager) throws DistSQLExcepti
String databaseName = yamlConfig.getDatabaseName();
checkDatabaseName(databaseName, file);
checkDataSource(yamlConfig.getDataSources(), file);
- updateResources(databaseName, yamlConfig.getDataSources());
- updateRules(databaseName, yamlConfig.getRules());
+ addDatabase(databaseName);
+ addResources(databaseName, yamlConfig.getDataSources());
+ try {
+ addRules(databaseName, yamlConfig.getRules());
+ } catch (DistSQLException ex) {
+ dropDatabase(databaseName);
+ throw new RuntimeException(ex.getMessage());
Review Comment:
Why is RuntimeException thrown?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] RaigorJiang merged pull request #19037: Refactor ImportDatabaseConfigurationHandler.
Posted by GitBox <gi...@apache.org>.
RaigorJiang merged PR #19037:
URL: https://github.com/apache/shardingsphere/pull/19037
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org