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.

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

Raise HeartbeatFailedException after missing maxMissedHeartbeats (new tuning param) consecutive heartbeats #111

Merged
merged 2 commits into from
Jan 28, 2024

Conversation

achilleasa
Copy link
Owner

This PR implements a suggestion from #106 (comment).

The client-side heartbeat implementation is modified via the
introduction of a new tuning parameter called maxMissedHeartbeats
(default: 3).

If heartbeats are enabled and the client sends maxMissedHeartbeats
consecutive heartbeat messages at the interval negotiated with the
broker without receiving any message (heartbeat or regular traffic)
back, the client assumes that the server is not available and will raise
a HeartbeatFailedException.

Fixes #106

This commit implements a suggestion from #106 (comment).

The client-side heartbeat implementation is modified via the
introduction of a new tuning parameter called `maxMissedHeartbeats`
(default: 3).

If heartbeats are enabled and the client sends `maxMissedHeartbeats`
consecutive heartbeat messages at the interval negotiated with the
broker without receiving any message (heartbeat or regular traffic)
back, the client assumes that the server is not available and will raise
a HeartbeatFailedException.
@achilleasa achilleasa force-pushed the track-missed-heartbeats branch from fd85301 to 9e52ca9 Compare January 4, 2024 06:00
Copy link

github-actions bot commented Jan 4, 2024

Coverage Status

coverage: 93.062%. remained the same
when pulling 9e52ca9 on track-missed-heartbeats
into 6cdc538 on master.

@rajmaniar
Copy link
Contributor

This implementation looks reasonable and it's similar to what we've been running successfully for about a month now. I'd like to propose an alternative implementation that's a bit simpler which I parked in this draft pr #114

@achilleasa
Copy link
Owner Author

TBH, I consider this implementation simpler as the timer will get correctly reset in all scenarios (re-connects e.t.c.). The in-flight tracking PR makes sense but needs more work to make sure that the counter gets correctly reset.

In the meantime I will land this CL so it does not bitrot.

@achilleasa achilleasa merged commit 019d59a into master Jan 28, 2024
1 check passed
@achilleasa achilleasa deleted the track-missed-heartbeats branch January 28, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Low traffic or publish-only connections are closed with "too many missed heartbeats"
2 participants