dolphinscheduler icon indicating copy to clipboard operation
dolphinscheduler copied to clipboard

[Feature-11129] [API] Refactor org.apache.dolphinscheduler.api.controller.ProcessDefinitionController

Open MichaelDeSteven opened this issue 3 years ago • 10 comments

Purpose of the pull request

close #11129

change log

  • refactor ProcessDefinitionService interface:use Result to replace Map<String, Object>
  • add WorkflowDefinitionController

MichaelDeSteven avatar Jul 27 '22 13:07 MichaelDeSteven

Codecov Report

Merging #11180 (093248c) into dev (181cd2f) will increase coverage by 0.00%. The diff coverage is 38.11%.

:exclamation: Current head 093248c differs from pull request most recent head 40011b9. Consider uploading reports for the commit 40011b9 to get more accurate results

@@            Coverage Diff            @@
##                dev   #11180   +/-   ##
=========================================
  Coverage     40.24%   40.24%           
- Complexity     4925     4927    +2     
=========================================
  Files           982      982           
  Lines         37558    37548   -10     
  Branches       4127     4124    -3     
=========================================
- Hits          15115    15113    -2     
+ Misses        20908    20902    -6     
+ Partials       1535     1533    -2     
Impacted Files Coverage Δ
...che/dolphinscheduler/api/python/PythonGateway.java 20.00% <0.00%> (+0.18%) :arrow_up:
.../apache/dolphinscheduler/common/utils/S3Utils.java 0.00% <0.00%> (ø)
...r/server/master/service/MasterFailoverService.java 54.47% <0.00%> (-2.31%) :arrow_down:
...api/service/impl/ProcessDefinitionServiceImpl.java 32.20% <40.55%> (+0.47%) :arrow_up:
...er/api/controller/ProcessDefinitionController.java 38.57% <45.71%> (-6.78%) :arrow_down:
...cheduler/api/service/impl/ExecutorServiceImpl.java 40.09% <50.00%> (+0.47%) :arrow_up:
...r/api/service/impl/ProcessInstanceServiceImpl.java 60.10% <50.00%> (ø)
...che/dolphinscheduler/alert/AlertSenderService.java 55.39% <56.25%> (+0.65%) :arrow_up:
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

codecov-commenter avatar Jul 28 '22 02:07 codecov-commenter

@caishunfeng @SbloodyS this pr is ready to review. plz take a look.

MichaelDeSteven avatar Jul 29 '22 07:07 MichaelDeSteven

I haven't seen any v2 controller in this PR. Is there something missing? @MichaelDeSteven

SbloodyS avatar Aug 02 '22 03:08 SbloodyS

I haven't seen any v2 controller in this PR. Is there something missing? @MichaelDeSteven

@SbloodyS the refactor work is so huge. I want to split the refactor task as small as possible. Hoping this way can make refactor task going well.
this pr is first step. After that, I will continue v2 controller task.

MichaelDeSteven avatar Aug 02 '22 05:08 MichaelDeSteven

I haven't seen any v2 controller in this PR. Is there something missing? @MichaelDeSteven

@SbloodyS the refactor work is so huge. I want to split the refactor task as small as possible. Hoping this way can make refactor task going well. this pr is first step. After that, I will continue v2 controller task.

So we split the refactor task to a single function. Each pr should contain at least one refactor function and its related modifications. It is not recommended to submit irrelevant things in advance.

SbloodyS avatar Aug 02 '22 07:08 SbloodyS

I haven't seen any v2 controller in this PR. Is there something missing? @MichaelDeSteven

@SbloodyS the refactor work is so huge. I want to split the refactor task as small as possible. Hoping this way can make refactor task going well. this pr is first step. After that, I will continue v2 controller task.

So we split the refactor task to a single function. Each pr should contain at least one refactor function and its related modifications. It is not recommended to submit irrelevant things in advance.

how about this, I will add some interface in v2 controller base on this pr. after the pr is merged, I will continue pr #11148 to finish the rest of interface?

MichaelDeSteven avatar Aug 02 '22 07:08 MichaelDeSteven

If you think the refactor work is too large. You can submit a PR for each refactor method. @MichaelDeSteven

SbloodyS avatar Aug 02 '22 07:08 SbloodyS

If you think the refactor work is too large. You can submit a PR for each refactor method. @MichaelDeSteven

the tough part is solved in this pr. I will complete v2 controller task in this pr. code review may take some time. but that would be all right.

MichaelDeSteven avatar Aug 02 '22 07:08 MichaelDeSteven

If you think the refactor work is too large. You can submit a PR for each refactor method. @MichaelDeSteven

the tough part is solved in this pr. I will complete v2 controller task in this pr. code review may take some time. but that would be all right.

In the mean time, you can convert this pr to draft. And mark it as ready to review after you finish your work.

SbloodyS avatar Aug 02 '22 07:08 SbloodyS

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

github-actions[bot] avatar Oct 24 '23 00:10 github-actions[bot]

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

github-actions[bot] avatar Oct 31 '23 00:10 github-actions[bot]