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/07 03:03:49 UTC
[GitHub] [shardingsphere] FlyingZC opened a new pull request, #18911: Support a Proxy instance uses one TM.
FlyingZC opened a new pull request, #18911:
URL: https://github.com/apache/shardingsphere/pull/18911
Changes proposed in this pull request:
- create one `TransactionManagerEngine` when proxy start;
- when add or remove resource, will rebuid the `TransactionManagerEngine`;
- when all of the resources are removed, will destory the `TransactionManagerEngine`;
--
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] FlyingZC commented on a diff in pull request #18911: Support a Proxy instance using one TM.
Posted by GitBox <gi...@apache.org>.
FlyingZC commented on code in PR #18911:
URL: https://github.com/apache/shardingsphere/pull/18911#discussion_r916502311
##########
shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/rule/TransactionRule.java:
##########
@@ -50,30 +51,35 @@ public final class TransactionRule implements GlobalRule, ResourceHeldRule<Shard
private final Map<String, ShardingSphereDatabase> databases;
- private final Map<String, ShardingSphereTransactionManagerEngine> resources;
+ private volatile ShardingSphereTransactionManagerEngine transactionManagerEngine;
public TransactionRule(final TransactionRuleConfiguration ruleConfig, final Map<String, ShardingSphereDatabase> databases, final InstanceContext instanceContext) {
configuration = ruleConfig;
defaultType = TransactionType.valueOf(ruleConfig.getDefaultType().toUpperCase());
providerType = ruleConfig.getProviderType();
props = ruleConfig.getProps();
this.databases = databases;
- resources = createTransactionManagerEngines(databases, instanceContext);
+ transactionManagerEngine = createTransactionManagerEngine(databases);
}
- private Map<String, ShardingSphereTransactionManagerEngine> createTransactionManagerEngines(final Map<String, ShardingSphereDatabase> databases, final InstanceContext instanceContext) {
- Map<String, ShardingSphereTransactionManagerEngine> result = new HashMap<>(databases.keySet().size(), 1);
+ private synchronized ShardingSphereTransactionManagerEngine createTransactionManagerEngine(final Map<String, ShardingSphereDatabase> databases) {
+ if (databases.size() == 0) {
+ return new ShardingSphereTransactionManagerEngine();
+ }
+ ShardingSphereTransactionManagerEngine result = new ShardingSphereTransactionManagerEngine();
+ Map<String, DataSource> dataSourceMap = new HashMap<>(databases.size());
+ DatabaseType databaseType = null;
for (Entry<String, ShardingSphereDatabase> entry : databases.entrySet()) {
- result.put(entry.getKey(), createTransactionManagerEngine(entry.getValue()));
+ dataSourceMap.putAll(entry.getValue().getResource().getDataSources());
+ databaseType = entry.getValue().getProtocolType();
}
+ result.init(databaseType, dataSourceMap, providerType);
return result;
}
- private ShardingSphereTransactionManagerEngine createTransactionManagerEngine(final ShardingSphereDatabase database) {
- ShardingSphereTransactionManagerEngine result = new ShardingSphereTransactionManagerEngine();
- ShardingSphereResource resource = database.getResource();
- result.init(resource.getDatabaseType(), resource.getDataSources(), providerType);
- return result;
+ @Override
+ public ShardingSphereTransactionManagerEngine getResource() {
+ return transactionManagerEngine;
Review Comment:
The variable name `transactionManagerEngine` is inconsistent with the `getResource()` method name, maybe it can't use `@Getter`.
--
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] jingshanglu commented on a diff in pull request #18911: Support a Proxy instance uses one TM.
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on code in PR #18911:
URL: https://github.com/apache/shardingsphere/pull/18911#discussion_r916494530
##########
shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/rule/TransactionRule.java:
##########
@@ -82,23 +88,39 @@ public synchronized void addResource(final ShardingSphereDatabase database) {
if (null == database) {
return;
}
- ShardingSphereTransactionManagerEngine previousEngine = resources.put(database.getName(), createTransactionManagerEngine(database));
- if (null != previousEngine) {
- closeEngine(previousEngine);
- }
+ databases.put(database.getName(), database);
+ rebuildEngine();
}
@Override
public synchronized void closeStaleResource(final String databaseName) {
- ShardingSphereTransactionManagerEngine engine = resources.remove(databaseName);
- if (null != engine) {
- closeEngine(engine);
+ if (!databases.containsKey(databaseName)) {
+ return;
}
+ databases.remove(databaseName);
+ rebuildEngine();
}
@Override
- public void closeStaleResources() {
- resources.values().forEach(this::closeEngine);
+ public synchronized void closeStaleResource() {
+ databases.clear();
+ closeEngine();
+ }
+
+ private void rebuildEngine() {
+ ShardingSphereTransactionManagerEngine previousEngine = transactionManagerEngine;
+ if (null != previousEngine) {
+ closeEngine(previousEngine);
+ }
+ transactionManagerEngine = createTransactionManagerEngine(databases);
+ }
+
+ private void closeEngine() {
+ ShardingSphereTransactionManagerEngine engine = transactionManagerEngine;
+ if (null != engine) {
+ transactionManagerEngine = new ShardingSphereTransactionManagerEngine();
+ closeEngine(engine);
Review Comment:
May need to close and then new.
##########
shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-type/shardingsphere-transaction-xa/shardingsphere-transaction-xa-core/src/main/java/org/apache/shardingsphere/transaction/xa/XAShardingSphereTransactionManager.java:
##########
@@ -121,6 +122,8 @@ public void close() throws Exception {
each.close();
}
cachedDataSources.clear();
- xaTransactionManagerProvider.close();
+ if (Objects.nonNull(xaTransactionManagerProvider)) {
+ xaTransactionManagerProvider.close();
+ }
Review Comment:
`xaTransactionManagerProvider` must not be empty, maybe it is ok to close `xaTransactionManagerProvider` directly .
##########
shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/rule/TransactionRule.java:
##########
@@ -50,30 +51,35 @@ public final class TransactionRule implements GlobalRule, ResourceHeldRule<Shard
private final Map<String, ShardingSphereDatabase> databases;
- private final Map<String, ShardingSphereTransactionManagerEngine> resources;
+ private volatile ShardingSphereTransactionManagerEngine transactionManagerEngine;
public TransactionRule(final TransactionRuleConfiguration ruleConfig, final Map<String, ShardingSphereDatabase> databases, final InstanceContext instanceContext) {
configuration = ruleConfig;
defaultType = TransactionType.valueOf(ruleConfig.getDefaultType().toUpperCase());
providerType = ruleConfig.getProviderType();
props = ruleConfig.getProps();
this.databases = databases;
- resources = createTransactionManagerEngines(databases, instanceContext);
+ transactionManagerEngine = createTransactionManagerEngine(databases);
}
- private Map<String, ShardingSphereTransactionManagerEngine> createTransactionManagerEngines(final Map<String, ShardingSphereDatabase> databases, final InstanceContext instanceContext) {
- Map<String, ShardingSphereTransactionManagerEngine> result = new HashMap<>(databases.keySet().size(), 1);
+ private synchronized ShardingSphereTransactionManagerEngine createTransactionManagerEngine(final Map<String, ShardingSphereDatabase> databases) {
+ if (databases.size() == 0) {
+ return new ShardingSphereTransactionManagerEngine();
+ }
+ ShardingSphereTransactionManagerEngine result = new ShardingSphereTransactionManagerEngine();
+ Map<String, DataSource> dataSourceMap = new HashMap<>(databases.size());
+ DatabaseType databaseType = null;
for (Entry<String, ShardingSphereDatabase> entry : databases.entrySet()) {
- result.put(entry.getKey(), createTransactionManagerEngine(entry.getValue()));
+ dataSourceMap.putAll(entry.getValue().getResource().getDataSources());
+ databaseType = entry.getValue().getProtocolType();
}
+ result.init(databaseType, dataSourceMap, providerType);
return result;
}
- private ShardingSphereTransactionManagerEngine createTransactionManagerEngine(final ShardingSphereDatabase database) {
- ShardingSphereTransactionManagerEngine result = new ShardingSphereTransactionManagerEngine();
- ShardingSphereResource resource = database.getResource();
- result.init(resource.getDatabaseType(), resource.getDataSources(), providerType);
- return result;
+ @Override
+ public ShardingSphereTransactionManagerEngine getResource() {
+ return transactionManagerEngine;
Review Comment:
The function may be redundant, `Getter` is enough.
--
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 #18911: Support a Proxy instance uses one TM.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18911:
URL: https://github.com/apache/shardingsphere/pull/18911#issuecomment-1177017720
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/18911?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 [#18911](https://codecov.io/gh/apache/shardingsphere/pull/18911?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a1d632f) into [master](https://codecov.io/gh/apache/shardingsphere/commit/0ccd3f867c600be06c993945d9aab88d5330da59?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0ccd3f8) will **decrease** coverage by `0.01%`.
> The diff coverage is `27.50%`.
```diff
@@ Coverage Diff @@
## master #18911 +/- ##
============================================
- Coverage 59.31% 59.29% -0.02%
- Complexity 2297 2298 +1
============================================
Files 3781 3781
Lines 54533 54547 +14
Branches 9224 9226 +2
============================================
- Hits 32348 32346 -2
- Misses 19463 19478 +15
- Partials 2722 2723 +1
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/18911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...shardingsphere/mode/metadata/MetaDataContexts.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUtbW9kZS9zaGFyZGluZ3NwaGVyZS1tb2RlLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL21vZGUvbWV0YWRhdGEvTWV0YURhdGFDb250ZXh0cy5qYXZh) | `16.66% <0.00%> (ø)` | |
| [...ardingsphere/transaction/rule/TransactionRule.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUta2VybmVsL3NoYXJkaW5nc3BoZXJlLXRyYW5zYWN0aW9uL3NoYXJkaW5nc3BoZXJlLXRyYW5zYWN0aW9uLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3RyYW5zYWN0aW9uL3J1bGUvVHJhbnNhY3Rpb25SdWxlLmphdmE=) | `21.56% <10.71%> (-8.99%)` | :arrow_down: |
| [...saction/xa/XAShardingSphereTransactionManager.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUta2VybmVsL3NoYXJkaW5nc3BoZXJlLXRyYW5zYWN0aW9uL3NoYXJkaW5nc3BoZXJlLXRyYW5zYWN0aW9uLXR5cGUvc2hhcmRpbmdzcGhlcmUtdHJhbnNhY3Rpb24teGEvc2hhcmRpbmdzcGhlcmUtdHJhbnNhY3Rpb24teGEtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvdHJhbnNhY3Rpb24veGEvWEFTaGFyZGluZ1NwaGVyZVRyYW5zYWN0aW9uTWFuYWdlci5qYXZh) | `67.50% <50.00%> (-1.74%)` | :arrow_down: |
| [...he/shardingsphere/mode/manager/ContextManager.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUtbW9kZS9zaGFyZGluZ3NwaGVyZS1tb2RlLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL21vZGUvbWFuYWdlci9Db250ZXh0TWFuYWdlci5qYXZh) | `67.98% <75.00%> (ø)` | |
| [...rdingsphere/transaction/ConnectionTransaction.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUta2VybmVsL3NoYXJkaW5nc3BoZXJlLXRyYW5zYWN0aW9uL3NoYXJkaW5nc3BoZXJlLXRyYW5zYWN0aW9uLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3RyYW5zYWN0aW9uL0Nvbm5lY3Rpb25UcmFuc2FjdGlvbi5qYXZh) | `50.00% <100.00%> (ø)` | |
| [...ication/jdbc/datasource/JDBCBackendDataSource.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC9jb21tdW5pY2F0aW9uL2pkYmMvZGF0YXNvdXJjZS9KREJDQmFja2VuZERhdGFTb3VyY2UuamF2YQ==) | `62.85% <100.00%> (ø)` | |
| [...dbc/transaction/JDBCBackendTransactionManager.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC9jb21tdW5pY2F0aW9uL2pkYmMvdHJhbnNhY3Rpb24vSkRCQ0JhY2tlbmRUcmFuc2FjdGlvbk1hbmFnZXIuamF2YQ==) | `50.70% <100.00%> (ø)` | |
| [.../common/updatable/AlterTransactionRuleHandler.java](https://codecov.io/gh/apache/shardingsphere/pull/18911/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-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L2Rpc3RzcWwvcmFsL2NvbW1vbi91cGRhdGFibGUvQWx0ZXJUcmFuc2FjdGlvblJ1bGVIYW5kbGVyLmphdmE=) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/18911?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/18911?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 [0ccd3f8...a1d632f](https://codecov.io/gh/apache/shardingsphere/pull/18911?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] FlyingZC commented on a diff in pull request #18911: Support a Proxy instance uses one TM.
Posted by GitBox <gi...@apache.org>.
FlyingZC commented on code in PR #18911:
URL: https://github.com/apache/shardingsphere/pull/18911#discussion_r916499463
##########
shardingsphere-kernel/shardingsphere-transaction/shardingsphere-transaction-core/src/main/java/org/apache/shardingsphere/transaction/rule/TransactionRule.java:
##########
@@ -82,23 +88,39 @@ public synchronized void addResource(final ShardingSphereDatabase database) {
if (null == database) {
return;
}
- ShardingSphereTransactionManagerEngine previousEngine = resources.put(database.getName(), createTransactionManagerEngine(database));
- if (null != previousEngine) {
- closeEngine(previousEngine);
- }
+ databases.put(database.getName(), database);
+ rebuildEngine();
}
@Override
public synchronized void closeStaleResource(final String databaseName) {
- ShardingSphereTransactionManagerEngine engine = resources.remove(databaseName);
- if (null != engine) {
- closeEngine(engine);
+ if (!databases.containsKey(databaseName)) {
+ return;
}
+ databases.remove(databaseName);
+ rebuildEngine();
}
@Override
- public void closeStaleResources() {
- resources.values().forEach(this::closeEngine);
+ public synchronized void closeStaleResource() {
+ databases.clear();
+ closeEngine();
+ }
+
+ private void rebuildEngine() {
+ ShardingSphereTransactionManagerEngine previousEngine = transactionManagerEngine;
+ if (null != previousEngine) {
+ closeEngine(previousEngine);
+ }
+ transactionManagerEngine = createTransactionManagerEngine(databases);
+ }
+
+ private void closeEngine() {
+ ShardingSphereTransactionManagerEngine engine = transactionManagerEngine;
+ if (null != engine) {
+ transactionManagerEngine = new ShardingSphereTransactionManagerEngine();
+ closeEngine(engine);
Review Comment:
OK
--
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] jingshanglu merged pull request #18911: Support a Proxy instance using one TM.
Posted by GitBox <gi...@apache.org>.
jingshanglu merged PR #18911:
URL: https://github.com/apache/shardingsphere/pull/18911
--
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