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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
