[Improvement-11773][api] Optimize the log printing of the api module according… by guowei-su · Pull Request #11782 · apache/dolphinscheduler

guowei-su · 2022-09-05T16:58:11Z

Purpose of the pull request

This pull request optimizes the log printing of the api module.

Brief change log

Add or modify log of the files in api/service/impl/, etc.
close [Improvement][api] Optimize the log printing of the api module according to the log specification. #11773
doc [Improvement-11481][docs] Add log specification document to contribution guidelines. #11484

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

SbloodyS

Please associate the issue and resolves conflicts.

guowei-su · 2022-09-06T02:47:06Z

issue #11773

...main/java/org/apache/dolphinscheduler/api/permission/ResourcePermissionCheckServiceImpl.java

...eduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/TenantServiceImpl.java

caishunfeng · 2022-09-06T07:02:17Z

Please add a doc of How to print logs better.

github-advanced-security

Found 11 potential problems in the proposed changes. Check the Files changed tab for more details.

codecov-commenter · 2022-09-11T13:40:40Z

Codecov Report

Merging #11782 (3e19c8c) into dev (511149f) will decrease coverage by 0.03%.
The diff coverage is 37.11%.

@@             Coverage Diff              @@
##                dev   #11782      +/-   ##
============================================
- Coverage     38.67%   38.64%   -0.04%     
- Complexity     4049     4062      +13     
============================================
  Files           999      999              
  Lines         36728    37339     +611     
  Branches       4276     4286      +10     
============================================
+ Hits          14204    14428     +224     
- Misses        20901    21277     +376     
- Partials       1623     1634      +11

Impacted Files	Coverage Δ
...olphinscheduler/api/audit/AuditPublishService.java	`34.61% <0.00%> (ø)`
...phinscheduler/api/controller/TenantController.java	`62.50% <ø> (ø)`
...rg/apache/dolphinscheduler/api/k8s/K8sManager.java	`58.53% <0.00%> (ø)`
...lphinscheduler/api/permission/PermissionCheck.java	`0.00% <0.00%> (ø)`
...nscheduler/api/service/impl/DqRuleServiceImpl.java	`59.18% <0.00%> (ø)`
...r/plugin/datasource/api/utils/DataSourceUtils.java	`0.00% <0.00%> (ø)`
...er/api/service/impl/TaskDefinitionServiceImpl.java	`22.40% <8.69%> (-1.51%)`	⬇️
...i/service/impl/ProcessTaskRelationServiceImpl.java	`21.32% <12.19%> (-1.13%)`	⬇️
...inscheduler/api/controller/ExecutorController.java	`22.22% <12.50%> (-0.86%)`	⬇️
...heduler/api/service/impl/SchedulerServiceImpl.java	`8.88% <13.51%> (+0.27%)`	⬆️
... and 33 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-advanced-security

Found 84 potential problems in the proposed changes. Check the Files changed tab for more details.

...src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessDefinitionServiceImpl.java

...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

...heduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ExecutorController.java

...main/java/org/apache/dolphinscheduler/api/permission/ResourcePermissionCheckServiceImpl.java

...heduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java

...duler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectServiceImpl.java

...nscheduler-api/src/main/java/org/apache/dolphinscheduler/api/permission/PermissionCheck.java

...src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessDefinitionServiceImpl.java

        if (projectCode == targetProjectCode) {
+            logger.warn("Project code is same as target project code, projectCode:{}.", projectCode);
            return result;
        }


github-advanced-security

Found 24 potential problems in the proposed changes. Check the Files changed tab for more details.

...duler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectServiceImpl.java

github-advanced-security

Found 18 potential problems in the proposed changes. Check the Files changed tab for more details.

guowei-su · 2022-09-13T13:43:40Z

Please add a doc of How to print logs better.

The doc refs this doc pr: Add log specification document to contribution guidelines.

guowei-su · 2022-09-13T13:47:06Z

PLAT @caishunfeng

...src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessDefinitionServiceImpl.java

brave-lee

+1

… to the log specification doc. - Resolve conflict.

… to the log specification doc. - fix ProcessDefinitionServiceImpl.java dependency

… to the log specification doc. - Accept proposed change in ResourcePermissionCheckServiceImpl.java

… to the log specification doc. - Update the UT case in TenantServiceTest.java, UsersServiceTest.java, TaskGroupServiceTest.java. - Update log printing in TaskGroupServiceImpl.java. - Fix NPE of log parameter in ExecutorServiceImpl.java when call python api.

… to the log specification doc. - Fix NPE of log in WorkerGroupServiceImpl.java. - Fix log parameter missing bug in ResourcesServiceImpl.java.

… to the log specification doc. - Fix bugs and handle some vulnerability codes.

… to the log specification doc. - Fix e2e User test.

… to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis.

… to the log specification doc. - Resolve import conflict.

… to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis.

sonarqubecloud · 2022-09-16T17:26:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
42 Code Smells

38.0% Coverage
2.3% Duplication

brave-lee

+1

zhongjiajie · 2022-09-17T12:30:25Z

I do not think this is a good idea, we should directly throw error instead of print log, and actually I already throw error in #11912

zhongjiajie · 2022-09-17T13:21:10Z

I do not think this is a good idea, we should directly throw error instead of print log, and actually I already throw error in #11912

We already have ExceptionHandleraccept ServiceException and package ServiceException code and message into Result object, and it uses in the controller layer

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/exceptions/ApiExceptionHandler.java

Lines 39 to 43 in ae6aa53

    
           @ExceptionHandler(ServiceException.class) 
        
           public Result exceptionHandler(ServiceException e, HandlerMethod hm) { 
        
               logger.error("ServiceException: ", e); 
        
               return new Result(e.getCode(), e.getMessage()); 
        
           }

so when we throw an exception in the service layer, we will catch it in the controller layer and return the new Result object. And we already have exists method in

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java

Lines 657 to 659 in d3a77c6

    
           if (!processInstance.getState().isFinished()) { 
        
               throw new ServiceException(PROCESS_INSTANCE_STATE_OPERATION_ERROR, processInstance.getName(), processInstance.getState(), "delete"); 
        
           }

.

when exception throw in L658

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java

Line 658 in d3a77c6

    
           throw new ServiceException(PROCESS_INSTANCE_STATE_OPERATION_ERROR, processInstance.getName(), processInstance.getState(), "delete");

controller will catch it and package into

Result(code=Status.PROCESS_INSTANCE_STATE_OPERATION_ERROR.getCode(), message=Status.PROCESS_INSTANCE_STATE_OPERATION_ERROR.getMsg())

guowei-su · 2022-09-17T13:44:59Z

I do not think this is a good idea, we should directly throw error instead of print log, and actually I already throw error in #11912

We already have ExceptionHandleraccept ServiceException and package ServiceException code and message into Result object, and it uses in the controller layer

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/exceptions/ApiExceptionHandler.java

Lines 39 to 43 in ae6aa53

@ExceptionHandler(ServiceException.class)

public Result exceptionHandler(ServiceException e, HandlerMethod hm) {

logger.error("ServiceException: ", e);

return new Result(e.getCode(), e.getMessage());

}

so when we throw an exception in the service layer, we will catch it in the controller layer and return the new Result object. And we already have exists method in

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java

Lines 657 to 659 in d3a77c6

if (!processInstance.getState().isFinished()) {

throw new ServiceException(PROCESS_INSTANCE_STATE_OPERATION_ERROR, processInstance.getName(), processInstance.getState(), "delete");

}

.
when exception throw in L658

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java

Line 658 in d3a77c6

throw new ServiceException(PROCESS_INSTANCE_STATE_OPERATION_ERROR, processInstance.getName(), processInstance.getState(), "delete");

controller will catch it and package into Result(code=Status.PROCESS_INSTANCE_STATE_OPERATION_ERROR.getCode(), message=Status.PROCESS_INSTANCE_STATE_OPERATION_ERROR.getMsg())

Ok, I probably get it. I will take this approach to regulate the api module logging and exceptions in the new pr. Also, I will describe this specification in the logging specification doc.

…according… (apache#11782) * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Resolve conflict. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - fix ProcessDefinitionServiceImpl.java dependency * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Accept proposed change in ResourcePermissionCheckServiceImpl.java * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Update the UT case in TenantServiceTest.java, UsersServiceTest.java, TaskGroupServiceTest.java. - Update log printing in TaskGroupServiceImpl.java. - Fix NPE of log parameter in ExecutorServiceImpl.java when call python api. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix NPE of log in WorkerGroupServiceImpl.java. - Fix log parameter missing bug in ResourcesServiceImpl.java. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix bugs and handle some vulnerability codes. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix bugs and handle some vulnerability codes. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix e2e User test. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Resolve import conflict. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis.

wen-hemin

Please confirm.

wen-hemin · 2022-09-21T02:33:06Z

...duler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectServiceImpl.java

+     * @param project project
+     * @return true if the user have permission to see the project, otherwise return false
+     */
+    private boolean checkReadPermission(User user, Project project) {


Please confirm whether it is referenced

Sorry, this method was not referenced and was incorrectly merged while I was handling the conflict.

…according… (apache#11782) * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Resolve conflict. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - fix ProcessDefinitionServiceImpl.java dependency * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Accept proposed change in ResourcePermissionCheckServiceImpl.java * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Update the UT case in TenantServiceTest.java, UsersServiceTest.java, TaskGroupServiceTest.java. - Update log printing in TaskGroupServiceImpl.java. - Fix NPE of log parameter in ExecutorServiceImpl.java when call python api. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix NPE of log in WorkerGroupServiceImpl.java. - Fix log parameter missing bug in ResourcesServiceImpl.java. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix bugs and handle some vulnerability codes. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix bugs and handle some vulnerability codes. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix e2e User test. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Resolve import conflict. * [DS-11773][api] Optimize the log printing of the api module according to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis.

guowei-su requested review from SbloodyS and caishunfeng as code owners September 5, 2022 16:58

github-actions bot added the backend label Sep 5, 2022

SbloodyS assigned guowei-su Sep 6, 2022

SbloodyS added the improvement make more easy to user or prompt friendly label Sep 6, 2022

SbloodyS added this to the 3.1.0 milestone Sep 6, 2022

SbloodyS reviewed Sep 6, 2022

View reviewed changes

guowei-su closed this Sep 6, 2022

guowei-su force-pushed the api branch from e83c1e4 to 45ebd52 Compare September 6, 2022 02:35

github-actions bot removed the backend label Sep 6, 2022

guowei-su reopened this Sep 6, 2022

github-actions bot added the backend label Sep 6, 2022

caishunfeng reviewed Sep 6, 2022

View reviewed changes

caishunfeng added the miss:docs missing documents in PR label Sep 6, 2022

guowei-su force-pushed the api branch from a0433aa to 794c968 Compare September 11, 2022 09:13

github-advanced-security bot found potential problems Sep 11, 2022

View reviewed changes

...duler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProjectServiceImpl.java Fixed Show fixed Hide fixed

guowei-su force-pushed the api branch 2 times, most recently from d69aca4 to 8c92b18 Compare September 13, 2022 08:12

github-advanced-security bot found potential problems Sep 13, 2022

View reviewed changes

brave-lee requested review from brave-lee and caishunfeng September 14, 2022 05:52

guowei-su force-pushed the api branch 2 times, most recently from c618a67 to 3e19c8c Compare September 15, 2022 09:35

github-advanced-security bot found potential problems Sep 15, 2022

View reviewed changes

...src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessDefinitionServiceImpl.java Fixed Show fixed Hide fixed

guowei-su force-pushed the api branch from 3e19c8c to f4849a4 Compare September 15, 2022 13:13

brave-lee previously approved these changes Sep 16, 2022

View reviewed changes

guowei-su added 11 commits September 16, 2022 19:21

[DS-11773][api] Optimize the log printing of the api module according…

0b4e829

… to the log specification doc. - Resolve conflict.

[DS-11773][api] Optimize the log printing of the api module according…

b8a764b

… to the log specification doc. - fix ProcessDefinitionServiceImpl.java dependency

[DS-11773][api] Optimize the log printing of the api module according…

283b93d

… to the log specification doc. - Accept proposed change in ResourcePermissionCheckServiceImpl.java

[DS-11773][api] Optimize the log printing of the api module according…

703a9e6

… to the log specification doc. - Fix NPE of log in WorkerGroupServiceImpl.java. - Fix log parameter missing bug in ResourcesServiceImpl.java.

[DS-11773][api] Optimize the log printing of the api module according…

30011ea

… to the log specification doc. - Fix bugs and handle some vulnerability codes.

[DS-11773][api] Optimize the log printing of the api module according…

a469163

… to the log specification doc. - Fix bugs and handle some vulnerability codes.

[DS-11773][api] Optimize the log printing of the api module according…

f47c2b9

… to the log specification doc. - Fix e2e User test.

[DS-11773][api] Optimize the log printing of the api module according…

1a5a5eb

… to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis.

[DS-11773][api] Optimize the log printing of the api module according…

3ee36e5

… to the log specification doc. - Resolve import conflict.

[DS-11773][api] Optimize the log printing of the api module according…

395b8f6

… to the log specification doc. - Fix codeQL check & SonarCloud Code Analysis.

guowei-su dismissed brave-lee’s stale review via 395b8f6 September 16, 2022 11:27

guowei-su force-pushed the api branch from f4849a4 to 395b8f6 Compare September 16, 2022 11:27

brave-lee approved these changes Sep 17, 2022

View reviewed changes

brave-lee merged commit 1a7c6ea into apache:dev Sep 17, 2022

zhongjiajie mentioned this pull request Sep 17, 2022

[Feature][API] New restful API for workflow and schedule #11912

Merged

caishunfeng modified the milestones: 3.1.0, 3.2.0 Sep 19, 2022

wen-hemin reviewed Sep 21, 2022

View reviewed changes

Conversation

guowei-su commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

guowei-su commented Sep 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caishunfeng commented Sep 6, 2022

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

guowei-su commented Sep 13, 2022

Uh oh!

guowei-su commented Sep 13, 2022

Uh oh!

Uh oh!

brave-lee left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 16, 2022

Uh oh!

brave-lee left a comment

Choose a reason for hiding this comment

Uh oh!

zhongjiajie commented Sep 17, 2022

Uh oh!

zhongjiajie commented Sep 17, 2022

Uh oh!

guowei-su commented Sep 17, 2022

Uh oh!

wen-hemin left a comment

Choose a reason for hiding this comment

Uh oh!

wen-hemin Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

guowei-su Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

guowei-su commented Sep 5, 2022 •

edited

Loading

codecov-commenter commented Sep 11, 2022 •

edited

Loading