Skip to content

[Fix-10827] Fix network error cause worker cannot send message to master#10886

Merged
ruanwenjun merged 3 commits intoapache:devfrom
ruanwenjun:dev_wenjun_fixNetwork
Jul 12, 2022
Merged

[Fix-10827] Fix network error cause worker cannot send message to master#10886
ruanwenjun merged 3 commits intoapache:devfrom
ruanwenjun:dev_wenjun_fixNetwork

Conversation

@ruanwenjun
Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun commented Jul 11, 2022 •
edited
Loading

Purpose of the pull request

close #10827

  • Rename some command to message, add sourceAddress and targetAddress in message
  • When worker send message to master use the separate connection
  • Add BaseMessage class, our message body should extends this class to set sourceAddress and target Address.

Brief change log

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)

This change added tests and can be verified as follows:

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixNetwork branch 5 times, most recently from 40588a9 to cbef534 Compare July 11, 2022 16:13
@ruanwenjun
Copy link
Copy Markdown
Member Author

ruanwenjun commented Jul 11, 2022

@caishunfeng This PR is ready to review, please take a look.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 11, 2022 •
edited
Loading

Codecov Report

Merging #10886 (4057d32) into dev (2f7281c) will decrease coverage by 0.02%.
The diff coverage is 1.76%.

@@             Coverage Diff              @@
##                dev   #10886      +/-   ##
============================================
- Coverage     40.21%   40.18%   -0.03%     
+ Complexity     4846     4840       -6     
============================================
  Files           940      945       +5     
  Lines         36982    36974       -8     
  Branches       4033     4045      +12     
============================================
- Hits          14871    14857      -14     
- Misses        20620    20621       +1     
- Partials       1491     1496       +5
Impacted Files Coverage Δ
...inscheduler/server/master/config/MasterConfig.java 33.33% <0.00%> (-1.67%) ⬇️
...ver/master/consumer/TaskPriorityQueueConsumer.java 0.00% <0.00%> (ø)
...master/dispatch/executor/NettyExecutorManager.java 0.00% <0.00%> (ø)
...ler/server/master/event/TaskDelayEventHandler.java 0.00% <0.00%> (ø)
...r/master/event/TaskRejectByWorkerEventHandler.java 0.00% <0.00%> (ø)
...er/server/master/event/TaskResultEventHandler.java 0.00% <0.00%> (ø)
...r/server/master/event/TaskRunningEventHandler.java 0.00% <0.00%> (ø)
...master/processor/TaskExecuteResponseProcessor.java 0.00% <0.00%> (ø)
.../master/processor/TaskExecuteRunningProcessor.java 25.00% <0.00%> (ø)
...r/server/master/processor/TaskRecallProcessor.java 0.00% <0.00%> (ø)
... and 43 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 2f7281c...4057d32. Read the comment docs.

@caishunfeng
Copy link
Copy Markdown
Contributor

caishunfeng commented Jul 12, 2022

Hi @ruanwenjun I found many changes in this pr is about command to message, is it necessary to change it?
I think we can keep the command name to reduce modification and cherry-pick costs, WDYT?

@ruanwenjun
Copy link
Copy Markdown
Member Author

ruanwenjun commented Jul 12, 2022 •
edited
Loading

Hi @ruanwenjun I found many changes in this pr is about command to message, is it necessary to change it?
I think we can keep the command name to reduce modification and cherry-pick costs, WDYT?

I add a new class BaseMessage, and our async command will extend this class, to get some fields.
I suggestion to rename command to message, since we already have a Command for netty, and the other command class is only the command body.

@SbloodyS SbloodyS added the bug Something isn't working label Jul 12, 2022
@SbloodyS SbloodyS added this to the 3.0.0-release milestone Jul 12, 2022
@ruanwenjun
Copy link
Copy Markdown
Member Author

ruanwenjun commented Jul 12, 2022

I will revert the name change related to command.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixNetwork branch from cbef534 to f525fb5 Compare July 12, 2022 02:24
@ruanwenjun
Copy link
Copy Markdown
Member Author

ruanwenjun commented Jul 12, 2022 •
edited
Loading

@caishunfeng I have reverted the rename change of command, please take a look.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fixNetwork branch from f525fb5 to 4057d32 Compare July 12, 2022 04:06
caishunfeng
caishunfeng reviewed Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM overall, some nip comments.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jul 12, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

2.5% 2.5% Coverage
0.4% 0.4% Duplication

@ruanwenjun ruanwenjun requested a review from caishunfeng July 12, 2022 05:55
caishunfeng
caishunfeng approved these changes Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

+1

@ruanwenjun ruanwenjun merged commit cade66a into apache:dev Jul 12, 2022
@ruanwenjun ruanwenjun deleted the dev_wenjun_fixNetwork branch July 12, 2022 06:08
ruanwenjun added a commit to ruanwenjun/dolphinscheduler that referenced this pull request Jul 12, 2022
@ruanwenjun
[Fix-10827] Fix network error cause worker cannot send message to mas…
109a4e8 
…ter (apache#10886)

* Fix network error cause worker cannot send message to master

(cherry picked from commit cade66a)
zhongjiajie pushed a commit to zhongjiajie/dolphinscheduler that referenced this pull request Jul 12, 2022
@ruanwenjun @zhongjiajie
[Fix-10827] Fix network error cause worker cannot send message to mas…
027488f 
…ter (apache#10886)

* Fix network error cause worker cannot send message to master
@ruanwenjun ruanwenjun mentioned this pull request Jul 13, 2022
Merged
ruanwenjun added a commit that referenced this pull request Jul 19, 2022
@ruanwenjun
[Fix-10827] Fix network error cause worker cannot send message to mas…
9ee20cf 
…ter (#10886)

* Fix network error cause worker cannot send message to master

(cherry picked from commit cade66a)
ruanwenjun added a commit to ruanwenjun/dolphinscheduler that referenced this pull request Aug 1, 2022
@ruanwenjun
[Fix-10827] Fix network error cause worker cannot send message to mas…
17c7f59 
…ter (apache#10886) (apache#21)

* Fix network error cause worker cannot send message to master

(cherry picked from commit cade66a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caishunfeng caishunfeng caishunfeng approved these changes

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

Assignees

@ruanwenjun ruanwenjun

Labels

backend bug Something isn't working

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

[Improvement][RPC] When client send request to server use a separate connection

4 participants

@ruanwenjun @codecov-commenter @caishunfeng @SbloodyS