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

[Improvement] rss.server.heartbeat.timeout should be replaced with rss.server.heartbeat.interval #927

Closed
3 tasks done
summaryzb opened this issue Jun 5, 2023 · 2 comments
Closed
3 tasks done

[Improvement] rss.server.heartbeat.timeout should be replaced with rss.server.heartbeat.interval #927

summaryzb opened this issue Jun 5, 2023 · 2 comments

Comments

@summaryzb
Copy link
Contributor

summaryzb commented Jun 5, 2023 •
edited
Loading

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

What would you like to be improved?

When rss.server.heartbeat.timeout such as 60 * 1000 is bigger than rss.server.heartbeat.interval such as 10 * 1000, let's assume that just one heartbeat request was very slow effected by the network delay.
The executorService startHeartBeat execute the action of sendHearBeat by rss.server.heartbeat.interval such as 10 * 1000 milliseconds.
The executorService sendHeartBeat execute the real action of hearBeat RPC request, however when the RPC request is timeout, the thread in the executorService sendHeartBeat will be blocked. since no more two thread is available, the following sending request will be in the blockingQueue.
As a result, the server only send one hearBeat request in 60 * 1000 milliseconds, that will lead coordinator delete the server, since the server heartBeat exceeding rss.coordinator.server.heartbeat.timeout.

How should we improve?

  1. rss.server.heartbeat.timeout should be no more than rss.server.heartbeat.interval.
  2. Eleminate rss.server.heartbeat.timeout replaced with rss.server.heartbeat.interval.

I preffer the second resolution.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@summaryzb summaryzb changed the title [Improvement] rss.server.heartbeat.timeout should be no more than rss.server.heartbeat.interval [Improvement] rss.server.heartbeat.timeout should be replaced with rss.server.heartbeat.interval Jun 5, 2023
@jerqi
Copy link
Contributor

jerqi commented Jun 5, 2023

Make sense. You can go ahead.

@summaryzb summaryzb mentioned this issue Jun 5, 2023
Merged
jerqi pushed a commit that referenced this issue Jun 5, 2023
@summaryzb
[#927] Improvement: improve the control of server heartbeat (#928)
0ea306f 
### What changes were proposed in this pull request?
 Eleminate rss.server.heartbeat.timeout replaced with rss.server.heartbeat.interval

### Why are the changes needed?
#927

### Does this PR introduce _any_ user-facing change?
Yes, `rss.server.heartbeat.timeout` will effect nothing

### How was this patch tested?
UnitTest
@jerqi
Copy link
Contributor

jerqi commented Jun 5, 2023

closed by #928

@jerqi jerqi closed this as completed Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jerqi @summaryzb