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/01/21 12:49:21 UTC
[GitHub] [shardingsphere] ReyYang opened a new pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
ReyYang opened a new pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991
Fixes #14722.
Changes proposed in this pull request:
- Add distributed lock
- Change Zookeeper distributed lock implementation
-
--
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] sandynz commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r789636393
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -75,8 +81,12 @@ public void ruleConfigurationCached(final RuleConfigurationCachedEvent event) {
String sourceRule = repository.get(SchemaMetaDataNode.getRulePath(event.getSchemaName()));
String targetRule = registryCacheManager.loadCache(SchemaMetaDataNode.getRulePath(event.getSchemaName()), event.getCacheId());
String ruleCacheId = event.getCacheId();
- StartScalingEvent startScalingEvent = new StartScalingEvent(event.getSchemaName(), sourceDataSource, sourceRule, targetRule, ruleCacheId);
- ShardingSphereEventBus.getInstance().post(startScalingEvent);
+ lockStatus = lockRegistryService.tryLock(event.getSchemaName(), 1000);
+ if (lockStatus) {
+ log.info("start scaling job, locked the schema name, event={}", event);
+ StartScalingEvent startScalingEvent = new StartScalingEvent(event.getSchemaName(), sourceDataSource, sourceRule, targetRule, ruleCacheId);
+ ShardingSphereEventBus.getInstance().post(startScalingEvent);
+ }
Review comment:
Could be
```
if (!lockStatus) {
return;
}
```
and keep original code unchanged.
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +108,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (lockStatus) {
Review comment:
From code, seems `ScalingTaskFinishedEvent` is not sent in every case, e.g. target DB broken and scaling job failed. The lock might be not released properly.
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -56,11 +57,16 @@
private final RegistryCacheManager registryCacheManager;
+ private final LockRegistryService lockRegistryService;
+
+ private boolean lockStatus;
Review comment:
Questions:
1. Is `voilatile` necessary?
2. Should `lockStatus` be isolated by every schema name?
3. Could `lockStatus` name be `locked` like?
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +108,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (lockStatus) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(event.getTargetSchemaName());
Review comment:
Should the lock name be decorated? e.g. scaling-schemaName.
--
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] terrymanu merged pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
terrymanu merged pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991
--
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] sandynz commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791436856
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +111,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (schemaNameLockedMap.getOrDefault(decorateLockName(event.getTargetSchemaName()), false)) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(event.getTargetSchemaName());
Review comment:
Seems `schemaNameLockedMap` just has `put` operation, but no `remove` operation, will it cause issue?
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +108,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (lockStatus) {
Review comment:
OK, then you could comment it in this PR, it's clear to see it later.
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -130,6 +127,20 @@ public void clusterSwitchConfiguration(final ClusterSwitchConfigurationEvent eve
persistService.persist(schemaName, event.getTargetRuleConfigs());
}
+ /**
+ * scaling release lock.
+ *
+ * @param event Scaling Job Release Schema Name Lock Event
+ */
+ @Subscribe
+ public void scalingJobReleaseLockEvent(final ScalingJobReleaseSchemaNameLockEvent event) {
+ if (schemaNameLockedMap.getOrDefault(event.getSchemaName(), false)) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(decorateLockName(event.getSchemaName()));
+ schemaNameLockedMap.remove(event.getSchemaName());
+ }
+ }
Review comment:
Is new event necessary for success case? Could we just do it in `scalingTaskFinished` like before, and add new logic for failure case later?
--
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] ReyYang commented on pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
ReyYang commented on pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#issuecomment-1020899385
There are lock release failures when the task is stopped abruptly. Here I will raise a new pull request to solve it
--
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] sandynz commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791590173
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -130,6 +127,20 @@ public void clusterSwitchConfiguration(final ClusterSwitchConfigurationEvent eve
persistService.persist(schemaName, event.getTargetRuleConfigs());
}
+ /**
+ * scaling release lock.
+ *
+ * @param event Scaling Job Release Schema Name Lock Event
+ */
+ @Subscribe
+ public void scalingJobReleaseLockEvent(final ScalingJobReleaseSchemaNameLockEvent event) {
+ if (schemaNameLockedMap.getOrDefault(event.getSchemaName(), false)) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(decorateLockName(event.getSchemaName()));
+ schemaNameLockedMap.remove(event.getSchemaName());
+ }
+ }
Review comment:
Is new event necessary for success case? Could we just do it in `scalingTaskFinished` like before, and add new logic for failure case later?
--
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] sandynz commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791436856
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +111,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (schemaNameLockedMap.getOrDefault(decorateLockName(event.getTargetSchemaName()), false)) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(event.getTargetSchemaName());
Review comment:
Seems `schemaNameLockedMap` just has `put` operation, but no `remove` operation, will it cause issue?
--
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] sandynz commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791437488
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +108,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (lockStatus) {
Review comment:
OK, then you could comment it in this PR, it's clear to see it later.
--
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] ReyYang commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
ReyYang commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791404767
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +108,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (lockStatus) {
Review comment:
Yes, there are lock release failures when the task is stopped abruptly. Here I will raise a new pull request to solve it
--
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] ReyYang commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
ReyYang commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791404767
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -98,6 +108,10 @@ public void scalingTaskFinished(final ScalingTaskFinishedEvent event) {
log.info("start to delete cache, ruleCacheId={}", ruleCacheId);
registryCacheManager.deleteCache(SchemaMetaDataNode.getRulePath(event.getTargetSchemaName()), ruleCacheId);
}
+ if (lockStatus) {
Review comment:
Yes, there are lock release failures when the task is stopped abruptly. Here I will raise a new pull request to solve it
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -130,6 +127,20 @@ public void clusterSwitchConfiguration(final ClusterSwitchConfigurationEvent eve
persistService.persist(schemaName, event.getTargetRuleConfigs());
}
+ /**
+ * scaling release lock.
+ *
+ * @param event Scaling Job Release Schema Name Lock Event
+ */
+ @Subscribe
+ public void scalingJobReleaseLockEvent(final ScalingJobReleaseSchemaNameLockEvent event) {
+ if (schemaNameLockedMap.getOrDefault(event.getSchemaName(), false)) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(decorateLockName(event.getSchemaName()));
+ schemaNameLockedMap.remove(event.getSchemaName());
+ }
+ }
Review comment:
ok, I will implement this feature in a new pull request
--
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] ReyYang commented on a change in pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
ReyYang commented on a change in pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#discussion_r791753349
##########
File path: shardingsphere-mode/shardingsphere-mode-type/shardingsphere-cluster-mode/shardingsphere-cluster-mode-core/src/main/java/org/apache/shardingsphere/mode/manager/cluster/coordinator/registry/cache/subscriber/ScalingRegistrySubscriber.java
##########
@@ -130,6 +127,20 @@ public void clusterSwitchConfiguration(final ClusterSwitchConfigurationEvent eve
persistService.persist(schemaName, event.getTargetRuleConfigs());
}
+ /**
+ * scaling release lock.
+ *
+ * @param event Scaling Job Release Schema Name Lock Event
+ */
+ @Subscribe
+ public void scalingJobReleaseLockEvent(final ScalingJobReleaseSchemaNameLockEvent event) {
+ if (schemaNameLockedMap.getOrDefault(event.getSchemaName(), false)) {
+ log.info("scaling job finished, release schema name lock, event = {}", event);
+ lockRegistryService.releaseLock(decorateLockName(event.getSchemaName()));
+ schemaNameLockedMap.remove(event.getSchemaName());
+ }
+ }
Review comment:
ok, I will implement this feature in a new pull request
--
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] ReyYang commented on pull request #14991: For #14722：Support only one scaling job could be executed at the same time for one schema
Posted by GitBox <gi...@apache.org>.
ReyYang commented on pull request #14991:
URL: https://github.com/apache/shardingsphere/pull/14991#issuecomment-1020899385
There are lock release failures when the task is stopped abruptly. Here I will raise a new pull request to solve it
--
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