You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@inlong.apache.org by GitBox <gi...@apache.org> on 2022/11/17 02:48:46 UTC
[GitHub] [inlong] fuweng11 opened a new pull request, #6564: [INLONG-6563][Manager] Fix the problem of no error message was displayed when deleting a group failed
fuweng11 opened a new pull request, #6564:
URL: https://github.com/apache/inlong/pull/6564
### Prepare a Pull Request
- Fixes #6563
### Motivation
Fix the problem of no error message was displayed when deleting a group failed
### Modifications
Get the execution result of deleting the group workflow, and throw an exception if the deletion fails
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #6564: [INLONG-6563][Manager] Fix the problem of no error message was displayed when deleting a group failed
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #6564:
URL: https://github.com/apache/inlong/pull/6564#discussion_r1024852544
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/group/InlongGroupProcessService.java:
##########
@@ -289,7 +292,13 @@ private void invokeDeleteProcess(String groupId, String operator) {
InlongGroupInfo groupInfo = groupService.doDeleteCheck(groupId, operator);
// start to delete group process
GroupResourceProcessForm form = genGroupResourceProcessForm(groupInfo, GroupOperateType.DELETE);
- workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
+ WorkflowResult result = workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
+ List<TaskResponse> tasks = result.getNewTasks();
+ if (TaskStatus.FAILED.equals(tasks.get(tasks.size() - 1).getStatus())) {
+ String errMsg = String.format("failed to delete group process for groupId=%s", groupId);
+ LOGGER.error(errMsg);
Review Comment:
1. We are all at least at the `info` level. No matter in the test or production environment, we use logs at the `info` level at least;
2. The following `WorkflowListenerException` also uses this error message, so it reuses the error message.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #6564: [INLONG-6563][Manager] Fix the problem of no error message was displayed when deleting a group failed
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #6564:
URL: https://github.com/apache/inlong/pull/6564#discussion_r1024854227
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/group/InlongGroupProcessService.java:
##########
@@ -289,7 +292,13 @@ private void invokeDeleteProcess(String groupId, String operator) {
InlongGroupInfo groupInfo = groupService.doDeleteCheck(groupId, operator);
// start to delete group process
GroupResourceProcessForm form = genGroupResourceProcessForm(groupInfo, GroupOperateType.DELETE);
- workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
+ WorkflowResult result = workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
+ List<TaskResponse> tasks = result.getNewTasks();
+ if (TaskStatus.FAILED == tasks.get(tasks.size() - 1).getStatus()) {
+ String errMsg = String.format("failed to delete group process for groupId=%s", groupId);
Review Comment:
```suggestion
String errMsg = String.format("failed to delete inlong group for groupId=%s", groupId);
```
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] luchunliang commented on a diff in pull request #6564: [INLONG-6563][Manager] Fix the problem of no error message was displayed when deleting a group failed
Posted by GitBox <gi...@apache.org>.
luchunliang commented on code in PR #6564:
URL: https://github.com/apache/inlong/pull/6564#discussion_r1024715767
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/group/InlongGroupProcessService.java:
##########
@@ -289,7 +292,13 @@ private void invokeDeleteProcess(String groupId, String operator) {
InlongGroupInfo groupInfo = groupService.doDeleteCheck(groupId, operator);
// start to delete group process
GroupResourceProcessForm form = genGroupResourceProcessForm(groupInfo, GroupOperateType.DELETE);
- workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
+ WorkflowResult result = workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
+ List<TaskResponse> tasks = result.getNewTasks();
+ if (TaskStatus.FAILED.equals(tasks.get(tasks.size() - 1).getStatus())) {
+ String errMsg = String.format("failed to delete group process for groupId=%s", groupId);
+ LOGGER.error(errMsg);
Review Comment:
It is better that
LOGGER.error("failed to delete group process for groupId={}", groupId);
when log level is fatal, errMsg object will not be created.
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on a diff in pull request #6564: [INLONG-6563][Manager] Fix the problem of no error message was displayed when deleting a group failed
Posted by GitBox <gi...@apache.org>.
gong commented on code in PR #6564:
URL: https://github.com/apache/inlong/pull/6564#discussion_r1024710777
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/group/InlongGroupProcessService.java:
##########
@@ -294,7 +294,7 @@ private void invokeDeleteProcess(String groupId, String operator) {
GroupResourceProcessForm form = genGroupResourceProcessForm(groupInfo, GroupOperateType.DELETE);
WorkflowResult result = workflowService.start(ProcessName.DELETE_GROUP_PROCESS, operator, form);
List<TaskResponse> tasks = result.getNewTasks();
- if (TaskStatus.FAILED == tasks.get(tasks.size() - 1).getStatus()) {
+ if (TaskStatus.FAILED.equals(tasks.get(tasks.size() - 1).getStatus())) {
Review Comment:
why not to use `== ` for enum
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong merged pull request #6564: [INLONG-6563][Manager] It should throw error info when deleting an inlong group failed
Posted by GitBox <gi...@apache.org>.
gong merged PR #6564:
URL: https://github.com/apache/inlong/pull/6564
--
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@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org