[GitHub] [shardingsphere] sandynz commented on a change in pull request #15407: For #14719: Fixed an issue where some DisSQL could cause errors when Scaling was not enabled

[GitBox <gi...@apache.org>]

sandynz commented on a change in pull request #15407:
URL: https://github.com/apache/shardingsphere/pull/15407#discussion_r806388418



##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/distsql/rdl/rule/RuleDefinitionBackendHandler.java
##########
@@ -68,6 +78,11 @@ protected ResponseHeader execute(final String schemaName, final T sqlStatement)
         ruleDefinitionUpdater.checkSQLStatement(shardingSphereMetaData, sqlStatement, currentRuleConfig);
         Optional<RuleDefinitionAlterPreprocessor> preprocessor = TypedSPIRegistry.findRegisteredService(RuleDefinitionAlterPreprocessor.class, sqlStatement.getClass().getCanonicalName(), 
                 new Properties());
+        if (!RuleAlteredJobWorker.isOnRuleAlteredActionEnabled(currentRuleConfig)) {
+            if (RULE_ALTERED_ACTION_LIST.contains(sqlStatement.getClass().getCanonicalName())) {
+                throw new RuntimeException("scaling is not enabled");
+            }
+        }
         if (RuleAlteredJobWorker.isOnRuleAlteredActionEnabled(currentRuleConfig) && preprocessor.isPresent()) {

Review comment:
       `RuleAlteredJobWorker.isOnRuleAlteredActionEnabled` is checked before, it's not necessary here. And it could use `else if`.

##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/distsql/rdl/rule/RuleDefinitionBackendHandler.java
##########
@@ -54,6 +61,9 @@
         ShardingSphereServiceLoader.register(RuleDefinitionAlterPreprocessor.class);
     }
     
+    private static final List<String> RULE_ALTERED_ACTION_LIST = Lists.newArrayList(AlterShardingTableRuleStatement.class.getName(), AlterShardingAlgorithmStatement.class.getName(),
+            AlterDefaultShardingStrategyStatement.class.getName(), AlterShardingBindingTableRulesStatement.class.getName(), AlterShardingBroadcastTableRulesStatement.class.getName());

Review comment:
       Why not use `Set`, since `contains` is used and it might be executed frequently.




-- 
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