You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2022/10/24 02:13:08 UTC
[GitHub] [dolphinscheduler] BongBongBang opened a new pull request, #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
BongBongBang opened a new pull request, #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499
Change query processDefinition one by one to batch during failover.
## Purpose of the pull request
<!--(For example: This pull request adds checkstyle plugin).-->
## Brief change log
<!--*(for example:)*
- *Add maven-checkstyle-plugin to root pom.xml*
-->
## Verify this pull request
<!--*(Please pick either of the following options)*-->
This pull request is code cleanup without any test coverage.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang closed pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang closed pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
URL: https://github.com/apache/dolphinscheduler/pull/12499
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] caishunfeng commented on pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
caishunfeng commented on PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#issuecomment-1290077626
> Migrate to #12506
Please keep one valid pr.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002628830
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
But how can i use this method in `MasterFailoverService`? Inject `ProcessDefinitionDaoImpl `class directly ? Does that fit in Dolphin's convention?
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002502591
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessServiceImpl.java:
##########
@@ -462,6 +462,40 @@ public ProcessDefinition findProcessDefinition(Long processDefinitionCode, int v
return processDefinition;
}
+ /**
+ * find a batch of process definitions by a map of <code, version>.
+ * @param codeVersionMap Map<code, version>
+ * @return
+ */
+ @Override
+ public List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap) {
+ Set<Long> codes = codeVersionMap.keySet();
+ List<ProcessDefinition> processDefinitions = processDefineMapper.queryByCodes(codes);
+ Map<Long, Integer> codeVersionNeedToRetrieve = new HashMap<>();
+ codeVersionNeedToRetrieve.putAll(codeVersionMap);
+ // filter out the code/version entry that don't need to retrieve
+ processDefinitions.forEach(processDefinition -> {
+ long code = processDefinition.getCode();
+ if (codeVersionNeedToRetrieve.containsKey(code) && processDefinition.getVersion() == codeVersionNeedToRetrieve.get(code).intValue()) {
+ codeVersionNeedToRetrieve.remove(code);
+ }
+ });
+ if (!codeVersionNeedToRetrieve.isEmpty()) {
+ List<ProcessDefinition> complementProcessDefinitions = codeVersionNeedToRetrieve.entrySet()
+ .stream()
+ .map(entry -> {
+ ProcessDefinition processDefinition = processDefineLogMapper.queryByDefinitionCodeAndVersion(entry.getKey(), entry.getValue());
+ if (processDefinition != null) {
+ processDefinition.setId(0);
+ }
+ return processDefinition;
+ })
+ .collect(Collectors.toList());
+ processDefinitions.addAll(complementProcessDefinitions);
+ }
+ return processDefinitions;
Review Comment:
Yeah, got it. It's a better idea.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a diff in pull request #12499: [Improvement][BatchQuery]Change query one by one to batch.
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002499243
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
Please don't add new method in `ProcessService` we will not maintain this class, you need to use ProcessDefinitionService of ProcessDefinitionLogService.
```suggestion
List<ProcessDefinition> findProcessDefinitionsByProcessInstances(List<ProcessInstance> processInstances);
```
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002629705
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
OKļ¼ I see the `DAO` layer packages the `Mapper` layer, it's kinda suitable for this purposeš
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002514972
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
I found that there is no `ProcessDefinitionLogService` in module `dolphinscheduler-service`.Do you mean that i need to create a new class in `dolphinscheduler-service` under package `org.apache.dolphinscheduler.service.process`?
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#issuecomment-1288313243
Migrate to #12506
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002629705
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
OKļ¼ I see the `DAO` layer assembles the `Mapper` layer, it's kinda suitable for this purposeš
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] caishunfeng closed pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
caishunfeng closed pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
URL: https://github.com/apache/dolphinscheduler/pull/12499
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002628455
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
Ok, I see the latest code, there is no `ProcessDefinitionLogService`, you can add `findProcessDefinitionsByProcessInstances(List<ProcessInstance> processInstances)` in `ProcessDefinitionDaoImpl` 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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] BongBongBang commented on a diff in pull request #12499: [Improvement][BatchQuery] Batch query ProcessDefinitions belongs to need failover ProcessInstance.
Posted by GitBox <gi...@apache.org>.
BongBongBang commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002628830
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
But how can i use this method in `MasterFailoverService`? Inject `ProcessDefinitionDaoImpl `class directly ? Is that suitable for Dolphin's convention?
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a diff in pull request #12499: [Improvement][BatchQuery]Change query one by one to batch.
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12499:
URL: https://github.com/apache/dolphinscheduler/pull/12499#discussion_r1002499243
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java:
##########
@@ -78,6 +75,8 @@ ProcessInstance handleCommand(String host,
ProcessDefinition findProcessDefinition(Long processDefinitionCode, int processDefinitionVersion);
+ List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap);
Review Comment:
```suggestion
List<ProcessDefinition> findProcessDefinitionsByProcessInstances(List<ProcessInstance> processInstances);
```
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessServiceImpl.java:
##########
@@ -462,6 +462,40 @@ public ProcessDefinition findProcessDefinition(Long processDefinitionCode, int v
return processDefinition;
}
+ /**
+ * find a batch of process definitions by a map of <code, version>.
+ * @param codeVersionMap Map<code, version>
+ * @return
+ */
+ @Override
+ public List<ProcessDefinition> findProcessDefinitions(Map<Long, Integer> codeVersionMap) {
+ Set<Long> codes = codeVersionMap.keySet();
+ List<ProcessDefinition> processDefinitions = processDefineMapper.queryByCodes(codes);
+ Map<Long, Integer> codeVersionNeedToRetrieve = new HashMap<>();
+ codeVersionNeedToRetrieve.putAll(codeVersionMap);
+ // filter out the code/version entry that don't need to retrieve
+ processDefinitions.forEach(processDefinition -> {
+ long code = processDefinition.getCode();
+ if (codeVersionNeedToRetrieve.containsKey(code) && processDefinition.getVersion() == codeVersionNeedToRetrieve.get(code).intValue()) {
+ codeVersionNeedToRetrieve.remove(code);
+ }
+ });
+ if (!codeVersionNeedToRetrieve.isEmpty()) {
+ List<ProcessDefinition> complementProcessDefinitions = codeVersionNeedToRetrieve.entrySet()
+ .stream()
+ .map(entry -> {
+ ProcessDefinition processDefinition = processDefineLogMapper.queryByDefinitionCodeAndVersion(entry.getKey(), entry.getValue());
+ if (processDefinition != null) {
+ processDefinition.setId(0);
+ }
+ return processDefinition;
+ })
+ .collect(Collectors.toList());
+ processDefinitions.addAll(complementProcessDefinitions);
+ }
+ return processDefinitions;
Review Comment:
Can we directly query from `processDefineLogMapper` by using below SQL?
```
select * from t_ds_process_definition_log where (code = xx and version = xx) or (code = xxx and version = xxx)
```
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org