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/08/08 08:51:16 UTC
[GitHub] [shardingsphere] huangdx0726 opened a new pull request, #19971: Fix19899
huangdx0726 opened a new pull request, #19971:
URL: https://github.com/apache/shardingsphere/pull/19971
Fixes #19899 .
Changes proposed in this pull request:
- In RuleAlteredJobScheduler, replace RuleAlteredJobContext to PipelineJobContext;
- Add InventoryTasks / IncrementalTasks / PipelineProcessContext in RuleAlteredJobScheduler;
--
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] huangdx0726 commented on a diff in pull request #19971: Decouple RuleAlteredJobScheduler and RuleAlteredJobContext for common usage
Posted by GitBox <gi...@apache.org>.
huangdx0726 commented on code in PR #19971:
URL: https://github.com/apache/shardingsphere/pull/19971#discussion_r940256535
##########
shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJob.java:
##########
@@ -80,7 +80,9 @@ public void execute(final ShardingContext shardingContext) {
return;
}
log.info("start RuleAlteredJobScheduler, jobId={}, shardingItem={}", jobId, shardingItem);
- RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext);
+ RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext, jobContext.getJobProcessContext());
Review Comment:
Could i move `PipelineProcessContext` to core module？ Then i can extractsome common methods from sub-class。
--
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 pull request #19971: Fix19899
Posted by GitBox <gi...@apache.org>.
sandynz commented on PR #19971:
URL: https://github.com/apache/shardingsphere/pull/19971#issuecomment-1208012637
@huangdx0726 , the PR title could be more meaningful, so it's easier to read in community.
--
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 diff in pull request #19971: Decouple RuleAlteredJobScheduler and RuleAlteredJobContext for common usage
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #19971:
URL: https://github.com/apache/shardingsphere/pull/19971#discussion_r940815752
##########
shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJob.java:
##########
@@ -80,7 +80,9 @@ public void execute(final ShardingContext shardingContext) {
return;
}
log.info("start RuleAlteredJobScheduler, jobId={}, shardingItem={}", jobId, shardingItem);
- RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext);
+ RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext, jobContext.getJobProcessContext());
Review Comment:
We could not move `PipelineProcessContext` to core module, since it's referenced in `PipelineJobContext`.
Since `ExecuteEngine` is in core module, could we ignore `PipelineProcessContext` and add inventory incremental execute engine?
--
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 diff in pull request #19971: Fix19899
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #19971:
URL: https://github.com/apache/shardingsphere/pull/19971#discussion_r940092627
##########
shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJob.java:
##########
@@ -80,7 +80,9 @@ public void execute(final ShardingContext shardingContext) {
return;
}
log.info("start RuleAlteredJobScheduler, jobId={}, shardingItem={}", jobId, shardingItem);
- RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext);
+ RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext, jobContext.getJobProcessContext());
+ jobScheduler.getInventoryTasks().addAll(jobContext.getInventoryTasks());
+ jobScheduler.getIncrementalTasks().addAll(jobContext.getIncrementalTasks());
Review Comment:
It's better to use constructor to initialize inventoryTasks and incrementalTasks.
##########
shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJobScheduler.java:
##########
@@ -17,17 +17,18 @@
package org.apache.shardingsphere.data.pipeline.scenario.rulealtered;
+import java.util.Collection;
+import java.util.LinkedList;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
Review Comment:
You could import code style schema `src/resources/code-style-idea.xml` into IDEA to keep standard importing.
##########
shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJob.java:
##########
@@ -80,7 +80,9 @@ public void execute(final ShardingContext shardingContext) {
return;
}
log.info("start RuleAlteredJobScheduler, jobId={}, shardingItem={}", jobId, shardingItem);
- RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext);
+ RuleAlteredJobScheduler jobScheduler = new RuleAlteredJobScheduler(jobContext, jobContext.getJobProcessContext());
Review Comment:
It might be not necessary to add `jobContext.getJobProcessContext()`, since we could get PipelineProcessContext from PipelineJobContext
##########
shardingsphere-kernel/shardingsphere-data-pipeline/shardingsphere-data-pipeline-core/src/main/java/org/apache/shardingsphere/data/pipeline/scenario/rulealtered/RuleAlteredJobScheduler.java:
##########
@@ -72,20 +79,7 @@ public void stop() {
public void run() {
String jobId = jobContext.getJobId();
GovernanceRepositoryAPI governanceRepositoryAPI = PipelineAPIFactory.getGovernanceRepositoryAPI();
- try {
- jobContext.getJobPreparer().prepare(jobContext);
- } catch (final PipelineIgnoredException ex) {
- log.info("pipeline ignore exception: {}", ex.getMessage());
- PipelineJobCenter.stop(jobId);
- // CHECKSTYLE:OFF
- } catch (final RuntimeException ex) {
- // CHECKSTYLE:ON
- log.error("job prepare failed, {}-{}", jobId, jobContext.getShardingItem(), ex);
- PipelineJobCenter.stop(jobId);
- jobContext.setStatus(JobStatus.PREPARING_FAILURE);
- governanceRepositoryAPI.persistJobProgress(jobContext);
- throw ex;
- }
+ // TODO prepare
Review Comment:
I've just merged an PR, the preparation code was moved, you could merge upstream master branch.
--
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 merged pull request #19971: Decouple RuleAlteredJobScheduler and RuleAlteredJobContext for common usage
Posted by GitBox <gi...@apache.org>.
sandynz merged PR #19971:
URL: https://github.com/apache/shardingsphere/pull/19971
--
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