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/09/19 13:17:32 UTC
[GitHub] [dolphinscheduler] DarkAssassinator opened a new pull request, #12051: [Improvement][Master] fix issue#12001
DarkAssassinator opened a new pull request, #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051
<!--Thanks very much for contributing to Apache DolphinScheduler. Please review https://dolphinscheduler.apache.org/en-us/community/development/pull-request.html before opening a pull request.-->
## Purpose of the pull request
fix issue #12001
## Brief change log
Just add a validator before task instance was put to dispatch queue.
Why need this change?
Because check the worker group before add to dispatch queue can avoid invalid infinite loops in `TaskPriorityQueueConsumer`
## Verify this pull request
--
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 #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r975954275
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/task/CommonTaskProcessor.java:
##########
@@ -126,6 +133,14 @@ public boolean dispatchTask() {
return false;
}
+ // check in advance to avoid invalid infinite loops in TaskPriorityQueueConsumer
+ if (CollectionUtils.isEmpty(serverNodeManager.getWorkerGroupNodes(taskExecutionContext.getWorkerGroup()))) {
Review Comment:
Add validation in API-Side can stop the process in workflow creation. In fact, we have this check in front-end, but in api-side, we don't validate the workgroup.
--
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] codecov-commenter commented on pull request #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#issuecomment-1251062051
# [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/12051?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#12051](https://codecov.io/gh/apache/dolphinscheduler/pull/12051?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (277dd7f) into [dev](https://codecov.io/gh/apache/dolphinscheduler/commit/8cddb10773fe8f7b58533b8e089cabc87622a039?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8cddb10) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## dev #12051 +/- ##
============================================
- Coverage 38.65% 38.63% -0.02%
+ Complexity 4005 4003 -2
============================================
Files 1002 1002
Lines 37182 37188 +6
Branches 4241 4242 +1
============================================
- Hits 14373 14369 -4
- Misses 21178 21186 +8
- Partials 1631 1633 +2
```
| [Impacted Files](https://codecov.io/gh/apache/dolphinscheduler/pull/12051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...server/master/runner/task/CommonTaskProcessor.java](https://codecov.io/gh/apache/dolphinscheduler/pull/12051/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1tYXN0ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9ydW5uZXIvdGFzay9Db21tb25UYXNrUHJvY2Vzc29yLmphdmE=) | `17.24% <0.00%> (-1.28%)` | :arrow_down: |
| [...e/dolphinscheduler/remote/NettyRemotingClient.java](https://codecov.io/gh/apache/dolphinscheduler/pull/12051/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1yZW1vdGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcmVtb3RlL05ldHR5UmVtb3RpbmdDbGllbnQuamF2YQ==) | `50.00% <0.00%> (-2.86%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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] DarkAssassinator commented on pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
DarkAssassinator commented on PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#issuecomment-1257218633
Misuse coverage in conflict resolution， close it and re-open
--
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] github-code-scanning[bot] commented on a diff in pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r979424866
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ExecutorServiceImpl.java:
##########
@@ -329,37 +371,37 @@
public boolean checkSubProcessDefinitionValid(ProcessDefinition processDefinition) {
// query all subprocesses under the current process
List<ProcessTaskRelation> processTaskRelations =
- processTaskRelationMapper.queryDownstreamByProcessDefinitionCode(processDefinition.getCode());
+ processTaskRelationMapper.queryDownstreamByProcessDefinitionCode(processDefinition.getCode());
if (processTaskRelations.isEmpty()) {
return true;
}
Set<Long> relationCodes =
- processTaskRelations.stream().map(ProcessTaskRelation::getPostTaskCode).collect(Collectors.toSet());
+ processTaskRelations.stream().map(ProcessTaskRelation::getPostTaskCode).collect(Collectors.toSet());
List<TaskDefinition> taskDefinitions = taskDefinitionMapper.queryByCodeList(relationCodes);
// find out the process definition code
Set<Long> processDefinitionCodeSet = new HashSet<>();
taskDefinitions.stream()
- .filter(task -> TaskConstants.TASK_TYPE_SUB_PROCESS.equalsIgnoreCase(task.getTaskType())).forEach(
- taskDefinition -> processDefinitionCodeSet.add(Long.valueOf(
- JSONUtils.getNodeString(taskDefinition.getTaskParams(),
- Constants.CMD_PARAM_SUB_PROCESS_DEFINE_CODE))));
+ .filter(task -> TaskConstants.TASK_TYPE_SUB_PROCESS.equalsIgnoreCase(task.getTaskType())).forEach(
+ taskDefinition -> processDefinitionCodeSet.add(Long.valueOf(
+ JSONUtils.getNodeString(taskDefinition.getTaskParams(),
+ Constants.CMD_PARAM_SUB_PROCESS_DEFINE_CODE))));
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/1542)
--
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] DarkAssassinator commented on a diff in pull request #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
DarkAssassinator commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r975420411
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/task/CommonTaskProcessor.java:
##########
@@ -126,6 +133,14 @@ public boolean dispatchTask() {
return false;
}
+ // check in advance to avoid invalid infinite loops in TaskPriorityQueueConsumer
+ if (CollectionUtils.isEmpty(serverNodeManager.getWorkerGroupNodes(taskExecutionContext.getWorkerGroup()))) {
Review Comment:
> We cannot make this change easily, the workgroup may loss in some cases, and recovery automically.
>
> And this is not safe, the workflow may exist in the processor but loss in dispatch part.
>
> BTY, it's not suggested to do this validation master, we need to do the validation in api-side.
but if ds want to worker group recovery automically, we also should not add this validation in api-side,just keep loop waiting. WDYT @ruanwenjun
--
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] sonarcloud[bot] commented on pull request #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#issuecomment-1251072078
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=12051)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_duplicated_lines_density&view=list)
--
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 #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r974821956
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/task/CommonTaskProcessor.java:
##########
@@ -126,6 +133,14 @@ public boolean dispatchTask() {
return false;
}
+ // check in advance to avoid invalid infinite loops in TaskPriorityQueueConsumer
+ if (CollectionUtils.isEmpty(serverNodeManager.getWorkerGroupNodes(taskExecutionContext.getWorkerGroup()))) {
Review Comment:
We cannot make this change easily, the workgroup may loss in some cases, and recovery automically.
And this is not safe, the workflow may exist in the processor but loss in dispatch part.
BTY, it's not suggested to do this validation master, we need to do the validation in api-side.
--
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 #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r975954275
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/task/CommonTaskProcessor.java:
##########
@@ -126,6 +133,14 @@ public boolean dispatchTask() {
return false;
}
+ // check in advance to avoid invalid infinite loops in TaskPriorityQueueConsumer
+ if (CollectionUtils.isEmpty(serverNodeManager.getWorkerGroupNodes(taskExecutionContext.getWorkerGroup()))) {
Review Comment:
Add validation in API-Side can stop the process in workflow creation.
--
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] DarkAssassinator commented on a diff in pull request #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
DarkAssassinator commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r977829796
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/task/CommonTaskProcessor.java:
##########
@@ -126,6 +133,14 @@ public boolean dispatchTask() {
return false;
}
+ // check in advance to avoid invalid infinite loops in TaskPriorityQueueConsumer
+ if (CollectionUtils.isEmpty(serverNodeManager.getWorkerGroupNodes(taskExecutionContext.getWorkerGroup()))) {
Review Comment:
> Add validation in API-Side can stop the process in workflow creation. In fact, we have this check in front-end, but in api-side, we don't validate the workgroup.
u are correct, i add a validator in api side and remove it in master side
--
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] github-code-scanning[bot] commented on a diff in pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r978752876
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ExecutorServiceImpl.java:
##########
@@ -314,11 +319,64 @@
ReleaseState.ONLINE.getDescp(), processDefineCode);
putMsg(result, Status.SUB_PROCESS_DEFINE_NOT_RELEASE);
} else {
- result.put(Constants.STATUS, Status.SUCCESS);
+ List<String> workerGroupNames = workerGroupService.getAllWorkerGroupNames();
+ return checkWorkerGroupNameExists(processDefinition, workerGroupNames);
}
return result;
}
+ /**
+ * check whether worker group is available
+ *
+ * @param processDefinition process definition
+ * @param workerGroupNames worker group name list
+ * @return check result
+ */
+ public Map<String, Object> checkWorkerGroupNameExists(ProcessDefinition processDefinition,
+ List<String> workerGroupNames) {
+ Map<String, Object> result = new HashMap<>();
+ // get all task definitions in this process definition
+ List<ProcessTaskRelation> processTaskRelations = processService
+ .findRelationByCode(processDefinition.getCode(), processDefinition.getVersion());
+ List<TaskDefinitionLog> taskDefinitionLogList = processService
+ .genTaskDefineList(processTaskRelations);
+ List<TaskDefinition> taskDefinitions = taskDefinitionLogList.stream()
+ .map(t -> (TaskDefinition) t).collect(
+ Collectors.toList());
+
+ for (TaskDefinition taskDefinition : taskDefinitions) {
+ if (!workerGroupNames.contains(taskDefinition.getWorkerGroup())) {
+ logger.error("Cannot find worker group {} configured on task definition named {} ",
+ taskDefinition.getWorkerGroup(), taskDefinition.getName());
+ putMsg(result, Status.WORKER_GROUP_NOT_EXISTS, taskDefinition.getName(),
+ taskDefinition.getWorkerGroup());
+ return result;
+ }
+
+ if (TaskConstants.TASK_TYPE_SUB_PROCESS
+ .equalsIgnoreCase(taskDefinition.getTaskType())) {
+ long subProcessCode = Long
+ .parseLong(JSONUtils.getNodeString(taskDefinition.getTaskParams(),
+ Constants.CMD_PARAM_SUB_PROCESS_DEFINE_CODE));
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/1539)
--
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 #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
caishunfeng commented on PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#issuecomment-1255706873
Hi @DarkAssassinator please use the correct title, thanks~
--
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] sonarcloud[bot] commented on pull request #12051: [Improvement][Master] fix issue#12001
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#issuecomment-1251082559
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=12051)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=12051&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=12051&metric=new_duplicated_lines_density&view=list)
--
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] DarkAssassinator commented on pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
DarkAssassinator commented on PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#issuecomment-1255724981
> Hi @DarkAssassinator please use the correct title, thanks~
ohh. so sorry. done
--
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] DarkAssassinator commented on a diff in pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
DarkAssassinator commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r978231655
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ExecutorServiceImpl.java:
##########
@@ -306,11 +311,64 @@ public Map<String, Object> checkProcessDefinitionValid(long projectCode, Process
logger.warn("Subprocess definition of process definition is not {}, processDefinitionCode:{}.", ReleaseState.ONLINE.getDescp(), processDefineCode);
putMsg(result, Status.SUB_PROCESS_DEFINE_NOT_RELEASE);
} else {
- result.put(Constants.STATUS, Status.SUCCESS);
+ List<String> workerGroupNames = workerGroupService.getAllWorkerGroupNames();
+ return checkWorkerGroupNameExists(processDefinition, workerGroupNames);
}
return result;
}
+ /**
+ * check whether worker group is available
+ *
+ * @param processDefinition process definition
+ * @param workerGroupNames worker group name list
+ * @return check result
+ */
+ public Map<String, Object> checkWorkerGroupNameExists(ProcessDefinition processDefinition,
+ List<String> workerGroupNames) {
+ Map<String, Object> result = new HashMap<>();
+ // get all task definitions in this process definition
+ List<ProcessTaskRelation> processTaskRelations = processService
+ .findRelationByCode(processDefinition.getCode(), processDefinition.getVersion());
+ List<TaskDefinitionLog> taskDefinitionLogList = processService
+ .genTaskDefineList(processTaskRelations);
+ List<TaskDefinition> taskDefinitions = taskDefinitionLogList.stream()
+ .map(t -> (TaskDefinition) t).collect(
+ Collectors.toList());
+
+ for (TaskDefinition taskDefinition : taskDefinitions) {
+ if (!workerGroupNames.contains(taskDefinition.getWorkerGroup())) {
+ logger.error("Cannot find worker group {} configured on task definition named {} ",
+ taskDefinition.getWorkerGroup(), taskDefinition.getName());
+ putMsg(result, Status.WORKER_GROUP_NOT_EXISTS, taskDefinition.getName(),
+ taskDefinition.getWorkerGroup());
+ return result;
+ }
+
+ if (TaskConstants.TASK_TYPE_SUB_PROCESS
+ .equalsIgnoreCase(taskDefinition.getTaskType())) {
+ long subProcessCode = Long
+ .parseLong(JSONUtils.getNodeString(taskDefinition.getTaskParams(),
+ Constants.CMD_PARAM_SUB_PROCESS_DEFINE_CODE));
+
+ ProcessDefinition subProcessDefinition = processDefinitionMapper
+ .queryByCode(subProcessCode);
+ if (subProcessDefinition == null) {
+ putMsg(result, Status.PROCESS_DEFINE_NOT_EXIST, String.valueOf(subProcessCode));
+ return result;
+ }
+ // check all sub process recursively
+ Map<String, Object> subResult = checkWorkerGroupNameExists(subProcessDefinition,
Review Comment:
> I am not sure if this will exist a cycle here.
because workflow is a DAG, so this change like a DFS, as i think it will not exist a cycle
--
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] DarkAssassinator closed pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
DarkAssassinator closed pull request #12051: [Improvement][Master] Add Worker Group validator in api side.
URL: https://github.com/apache/dolphinscheduler/pull/12051
--
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 #12051: [Improvement][Master] Add Worker Group validator in api side.
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #12051:
URL: https://github.com/apache/dolphinscheduler/pull/12051#discussion_r978221862
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ExecutorServiceImpl.java:
##########
@@ -306,11 +311,64 @@ public Map<String, Object> checkProcessDefinitionValid(long projectCode, Process
logger.warn("Subprocess definition of process definition is not {}, processDefinitionCode:{}.", ReleaseState.ONLINE.getDescp(), processDefineCode);
putMsg(result, Status.SUB_PROCESS_DEFINE_NOT_RELEASE);
} else {
- result.put(Constants.STATUS, Status.SUCCESS);
+ List<String> workerGroupNames = workerGroupService.getAllWorkerGroupNames();
+ return checkWorkerGroupNameExists(processDefinition, workerGroupNames);
}
return result;
}
+ /**
+ * check whether worker group is available
+ *
+ * @param processDefinition process definition
+ * @param workerGroupNames worker group name list
+ * @return check result
+ */
+ public Map<String, Object> checkWorkerGroupNameExists(ProcessDefinition processDefinition,
+ List<String> workerGroupNames) {
+ Map<String, Object> result = new HashMap<>();
+ // get all task definitions in this process definition
+ List<ProcessTaskRelation> processTaskRelations = processService
+ .findRelationByCode(processDefinition.getCode(), processDefinition.getVersion());
+ List<TaskDefinitionLog> taskDefinitionLogList = processService
+ .genTaskDefineList(processTaskRelations);
+ List<TaskDefinition> taskDefinitions = taskDefinitionLogList.stream()
+ .map(t -> (TaskDefinition) t).collect(
+ Collectors.toList());
+
+ for (TaskDefinition taskDefinition : taskDefinitions) {
+ if (!workerGroupNames.contains(taskDefinition.getWorkerGroup())) {
+ logger.error("Cannot find worker group {} configured on task definition named {} ",
+ taskDefinition.getWorkerGroup(), taskDefinition.getName());
+ putMsg(result, Status.WORKER_GROUP_NOT_EXISTS, taskDefinition.getName(),
+ taskDefinition.getWorkerGroup());
+ return result;
+ }
+
+ if (TaskConstants.TASK_TYPE_SUB_PROCESS
+ .equalsIgnoreCase(taskDefinition.getTaskType())) {
+ long subProcessCode = Long
+ .parseLong(JSONUtils.getNodeString(taskDefinition.getTaskParams(),
+ Constants.CMD_PARAM_SUB_PROCESS_DEFINE_CODE));
+
+ ProcessDefinition subProcessDefinition = processDefinitionMapper
+ .queryByCode(subProcessCode);
+ if (subProcessDefinition == null) {
+ putMsg(result, Status.PROCESS_DEFINE_NOT_EXIST, String.valueOf(subProcessCode));
+ return result;
+ }
+ // check all sub process recursively
+ Map<String, Object> subResult = checkWorkerGroupNameExists(subProcessDefinition,
Review Comment:
I am not sure if this will exist a cycle here.
--
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