[GitHub] [shardingsphere] sandynz commented on a change in pull request #15571: For #12449: If a scaling job failed or stopped then it could not be started by DistSQL except restarting proxy

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

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



##########
File path: shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/core/execute/PipelineJobExecutor.java
##########
@@ -105,6 +103,16 @@ private void execute(final JobConfigurationPOJO jobConfigPOJO) {
         }
     }
     
+    /**
+     * stop job.
+     * @param jobId job id
+     */
+    public void stopJob(final String jobId) {
+        log.info("remove and stop {}", jobId);
+        EXECUTING_JOBS.remove(jobId);
+        RuleAlteredJobSchedulerCenter.stop(jobId);
+    }

Review comment:
       Looks `EXECUTING_JOBS` is not needed any more, could we just depend on `RuleAlteredJobSchedulerCenter.stop` and remove this new method.

##########
File path: shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJobWorker.java
##########
@@ -86,6 +87,9 @@
     
     private static final AtomicBoolean WORKER_INITIALIZED = new AtomicBoolean(false);
     
+    @Getter
+    private static final PipelineJobExecutor PIPELINE_JOB_EXECUTOR = new PipelineJobExecutor();
+    

Review comment:
       It's not necessary to add `PIPELINE_JOB_EXECUTOR`.

##########
File path: shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJob.java
##########
@@ -53,7 +53,7 @@ public void execute(final ShardingContext shardingContext) {
         } catch (final RuntimeException ex) {
             // CHECKSTYLE:ON
             log.error("job prepare failed, {}-{}", shardingContext.getJobName(), shardingContext.getShardingItem());
-            jobContext.close();
+            RuleAlteredJobWorker.getPIPELINE_JOB_EXECUTOR().stopJob(shardingContext.getJobName());

Review comment:
       If `jobContext.close();` removed, looks it might cause resource leak.
   
   `RuleAlteredJobWorker.getPIPELINE_JOB_EXECUTOR().` could be replaced by `RuleAlteredJobSchedulerCenter.stop`.

##########
File path: shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/core/execute/PipelineJobExecutor.java
##########
@@ -105,6 +103,16 @@ private void execute(final JobConfigurationPOJO jobConfigPOJO) {
         }
     }
     
+    /**
+     * stop job.

Review comment:
       Illegal javadoc format.




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