Skip to content

add break & default logic for NettyDecoder.decode#2969

Merged
gabry-lab merged 5 commits intoapache:devfrom
superScala:fixNettyRemotingServer
Jun 16, 2020
Merged

add break & default logic for NettyDecoder.decode#2969
gabry-lab merged 5 commits intoapache:devfrom
superScala:fixNettyRemotingServer

Conversation

@superScala
Copy link
Copy Markdown
Contributor

@superScala superScala commented Jun 14, 2020

What is the purpose of the pull request

We should always add break & default logic to SWITCH , if one method using switch doesn't need to use break & default actually , the logic will be so hard to understand !!!

Brief change log

add break & default logic for NettyDecoder.decode

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 14, 2020 •
edited
Loading

Codecov Report

Merging #2969 into dev will decrease coverage by 0.18%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #2969      +/-   ##
============================================
- Coverage     39.46%   39.27%   -0.19%     
+ Complexity     2705     2698       -7     
============================================
  Files           435      435              
  Lines         20116    20123       +7     
  Branches       2453     2453              
============================================
- Hits           7938     7904      -34     
- Misses        11424    11467      +43     
+ Partials        754      752       -2
Impacted Files Coverage Δ Complexity Δ
...he/dolphinscheduler/remote/codec/NettyDecoder.java 58.53% <71.42%> (-26.76%) 7.00 <1.00> (+2.00) ⬇️
...he/dolphinscheduler/common/enums/SqoopJobType.java 0.00% <0.00%> (-88.89%) 0.00% <0.00%> (-3.00%)
...g/apache/dolphinscheduler/remote/command/Pong.java 0.00% <0.00%> (-87.50%) 0.00% <0.00%> (-2.00%)
...dolphinscheduler/remote/command/CommandHeader.java 70.00% <0.00%> (-30.00%) 4.00% <0.00%> (-3.00%)
...inscheduler/remote/handler/NettyServerHandler.java 23.40% <0.00%> (-25.54%) 3.00% <0.00%> (-2.00%)
...inscheduler/remote/handler/NettyClientHandler.java 20.40% <0.00%> (-22.45%) 2.00% <0.00%> (-3.00%)
...org/apache/dolphinscheduler/remote/utils/Pair.java 36.36% <0.00%> (-18.19%) 1.00% <0.00%> (-2.00%)
...pache/dolphinscheduler/remote/command/Command.java 37.93% <0.00%> (-17.25%) 7.00% <0.00%> (-2.00%)
...dolphinscheduler/remote/future/ResponseFuture.java 72.88% <0.00%> (-8.48%) 17.00% <0.00%> (-1.00%)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7ea4f1...f88d8a1. Read the comment docs.

@gabry-lab
Copy link
Copy Markdown
Member

gabry-lab commented Jun 15, 2020

@superScala Could you resolve the bad code smells ?

@gabry-lab
Copy link
Copy Markdown
Member

gabry-lab commented Jun 15, 2020 •
edited
Loading

@superScala I think this PR is OK, we'd better invite owner of this file @qiaozhanwei to review . If he has no suggestions , I will merge this PR.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jun 15, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

71.4% 71.4% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

gabry-lab
gabry-lab approved these changes Jun 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gabry-lab gabry-lab left a comment

Choose a reason for hiding this comment

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

+1

@gabry-lab gabry-lab merged commit 8031c16 into apache:dev Jun 16, 2020
@gabry-lab
Copy link
Copy Markdown
Member

gabry-lab commented Jun 16, 2020

@superScala Thanks for your contributions . Since no others have an opposite options ,I merged this PR

@davidzollo davidzollo added the enhancement New feature or request label Jun 18, 2020
qiaozhanwei pushed a commit to qiaozhanwei/incubator-dolphinscheduler that referenced this pull request Jun 22, 2020
revert apache#2969
adfdddf
@qiaozhanwei qiaozhanwei mentioned this pull request Jun 22, 2020
Merged
@gabry-lab
Copy link
Copy Markdown
Member

gabry-lab commented Jul 14, 2020 •
edited
Loading

hi ,@superScala , unfortunately @qiaozhanwei reverted this PR (#3035) and he gave a useful blog https://www.jianshu.com/p/1f27eaea611d to explain it, you can get infos from the blog.
According to the blog 'break' is no needed, however I till think it's not the best practice, or even ugly to remove break from switch. I suggest that you refactor it using Chain of Responsibility Pattern, waiting for your new PR about this

@CalvinKirs CalvinKirs mentioned this pull request Aug 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gabry-lab gabry-lab gabry-lab approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@superScala @codecov-commenter @gabry-lab @davidzollo