Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jul 12, 2022
Merged

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

merged 3 commits into from
Jul 12, 2022

Conversation

ruanwenjun
Copy link
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
Member Author

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

@codecov-commenter
Copy link

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
Contributor

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
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
Member Author

I will revert the name change related to command.

@ruanwenjun
Copy link
Member Author

ruanwenjun commented Jul 12, 2022 •
edited
Loading

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

caishunfeng
caishunfeng reviewed Jul 12, 2022
View reviewed changes
Copy link
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.

...in/java/org/apache/dolphinscheduler/server/master/processor/TaskExecuteRunningProcessor.java Show resolved Hide resolved
.../src/test/java/org/apache/dolphinscheduler/server/master/processor/TaskAckProcessorTest.java Show resolved Hide resolved
...er-worker/src/main/java/org/apache/dolphinscheduler/server/worker/message/MessageSender.java Show resolved Hide resolved
...r/src/main/java/org/apache/dolphinscheduler/server/worker/processor/TaskCallbackService.java Outdated Show resolved Hide resolved
...-worker/src/main/java/org/apache/dolphinscheduler/server/worker/rpc/WorkerMessageSender.java Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud 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

caishunfeng
caishunfeng approved these changes Jul 12, 2022
View reviewed changes
Copy link
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