Skip to content

[Feature][Api] Refactor org.apache.dolphinscheduler.api.controller.DataAnalysisController#12969

Closed
Zzihh wants to merge 9 commits intoapache:devfrom
Zzihh:data-analysis-v2
Closed

[Feature][Api] Refactor org.apache.dolphinscheduler.api.controller.DataAnalysisController#12969
Zzihh wants to merge 9 commits intoapache:devfrom
Zzihh:data-analysis-v2

Conversation

@Zzihh
Copy link
Copy Markdown
Contributor

@Zzihh Zzihh commented Nov 22, 2022

Purpose of the pull request

Fix #11754

zhanziheng and others added 4 commits September 5, 2022 18:12
Feature ：Refactor org.apache.dolphinscheduler.api.controller.DataAnal…
ef027ea 
…ysisController
Feature ：Refactor org.apache.dolphinscheduler.api.controller.DataAnal…
d47f3f1 
…ysisController
Feature ：Refactor org.apache.dolphinscheduler.api.controller.DataAnal…
ed44bf7 
…ysisController
Merge remote-tracking branch 'origin/data-analysis-v2' into data-anal…
d81181f 
…ysis-v2

# Conflicts:
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/DataAnalysisController.java
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/DataAnalysisService.java
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/DataAnalysisServiceImpl.java
#	dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/controller/DataAnalysisControllerTest.java
#	dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/DataAnalysisServiceTest.java
@Zzihh
Copy link
Copy Markdown
Contributor Author

Zzihh commented Nov 22, 2022

@SbloodyS Just accidentally closed the last PR.rebuild this.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 22, 2022
View reviewed changes
...scheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/DataAnalysisService.java
* @return queue state count data
*/
Map<String, Object> countQueueState(User loginUser);
Result countQueueState(User loginUser);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'loginUser' is never used.
@DarkAssassinator
Copy link
Copy Markdown
Contributor

DarkAssassinator commented Nov 22, 2022

parameters -> DTO is a good refactor, but why all service return still keep Result? I think that Result is a bad partice for service impl. If failed, service just need throw ServiceException, no need update the Result, such as f7caa6a. WDYT. cc @SbloodyS @caishunfeng

@EricGao888 EricGao888 added improvement make more easy to user or prompt friendly priority:middle 4.0.0 labels Nov 23, 2022
@EricGao888 EricGao888 added this to the 4.0.0-alpha milestone Nov 23, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 24, 2022 •
edited
Loading

Codecov Report

Merging #12969 (a7d354d) into dev (615b1db) will increase coverage by 0.04%.
The diff coverage is 90.24%.

❗ Current head a7d354d differs from pull request most recent head 9200317. Consider uploading reports for the commit 9200317 to get more accurate results

@@             Coverage Diff              @@
##                dev   #12969      +/-   ##
============================================
+ Coverage     39.61%   39.65%   +0.04%     
- Complexity     4356     4373      +17     
============================================
  Files          1097     1103       +6     
  Lines         41164    41198      +34     
  Branches       4716     4717       +1     
============================================
+ Hits          16306    16339      +33     
- Misses        23045    23052       +7     
+ Partials       1813     1807       -6
Impacted Files Coverage Δ
...uler/api/controller/v2/StatisticsV2Controller.java 86.36% <66.66%> (+2.36%) ⬆️
...uler/api/service/impl/DataAnalysisServiceImpl.java 39.92% <79.41%> (+3.43%) ⬆️
...heduler/api/controller/DataAnalysisController.java 100.00% <100.00%> (ø)
...duler/api/controller/DataAnalysisV2Controller.java 100.00% <100.00%> (ø)
...pi/dto/dataAnalysis/CommandStateCountResponse.java 100.00% <100.00%> (ø)
...aAnalysis/ProcessDefinitionStateCountResponse.java 100.00% <100.00%> (ø)
...ataAnalysis/ProcessInstanceStateCountResponse.java 100.00% <100.00%> (ø)
.../api/dto/dataAnalysis/QueueStateCountResponse.java 100.00% <100.00%> (ø)
...r/api/dto/dataAnalysis/TaskStateCountResponse.java 100.00% <100.00%> (ø)
...r/plugin/registry/zookeeper/ZookeeperRegistry.java 43.54% <0.00%> (-6.46%) ⬇️
... and 4 more

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

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Nov 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 55 Code Smells

98.8% 98.8% Coverage
0.0% 0.0% Duplication

@Zzihh
Copy link
Copy Markdown
Contributor Author

Zzihh commented Nov 24, 2022 •
edited
Loading

@SbloodyS @caishunfeng hello , Please help review that.

@SbloodyS SbloodyS mentioned this pull request Nov 25, 2022
Closed
3 tasks
@davidzollo davidzollo added the Waiting for review Waiting for review label Nov 27, 2022
Merge branch 'dev' into data-analysis-v2
5d41b36 
# Conflicts:
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/DataAnalysisService.java
@Zzihh Zzihh closed this Feb 2, 2023
@Zzihh Zzihh reopened this Feb 2, 2023
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Feb 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 57 Code Smells

89.4% 89.4% Coverage
0.0% 0.0% Duplication

@Zzihh
Copy link
Copy Markdown
Contributor Author

Zzihh commented Feb 2, 2023

@caishunfeng @SbloodyS Please help review that. please please please!!

@Zzihh Zzihh requested review from CalvinKirs and removed request for SbloodyS and caishunfeng February 3, 2023 03:05
davidzollo
davidzollo approved these changes Feb 3, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Feb 15, 2023

Result should be encapsulated in Collonter layer, not service layer

I agree with Calvin, we should avoid returning simple object instead of results in service, especially since your patch is refactoring for our codebase, we should try to make it better

@Zzihh Zzihh closed this by deleting the head repository May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidzollo davidzollo davidzollo approved these changes

@CalvinKirs CalvinKirs Awaiting requested review from CalvinKirs

Assignees

@caishunfeng caishunfeng

@Zzihh Zzihh

Labels

backend improvement make more easy to user or prompt friendly priority:middle Waiting for review Waiting for review

Projects

None yet

Milestone

4.0.0-alpha

Development

Successfully merging this pull request may close these issues.

[Feature][Api] Refactor org.apache.dolphinscheduler.api.controller.DataAnalysisController

9 participants

@Zzihh @DarkAssassinator @codecov-commenter @zhongjiajie @davidzollo @CalvinKirs @github-advanced-security @caishunfeng @EricGao888