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

Regenerate heartbeat by setting heartbeat_liveness instead of HEARTBEAT_LIVENESS #2977

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tdadela
Copy link
Contributor

@tdadela tdadela commented Nov 9, 2024

Currently, the heartbeat_liveness parameter is used only to initialize value self.heartbeat.
After receiving a heartbeat value is set to HEARTBEAT_LIVENESS: c.heartbeat = HEARTBEAT_LIVENESS.

I guess that original motivation to add this parameter was to give possibility to adjust how many missing heartbeats WorkerNode is considered missing.
To obtain this aim, value after regeneration should be equal to value passed to the constructor.

Another possible solution: complete removal of the heartbeat_liveness parameter from constructor.

Motivation behind change self._local_worker_node = WorkerNode(id="local", heartbeat_liveness=HEARTBEAT_LIVENESS) — default value of constructor parameter is evaluated during function creation but value HEARTBEAT_LIVENESS can change after that.

@cyberw
Copy link
Collaborator

cyberw commented Nov 17, 2024

I like what you're doing, but would prefer we rewrite it so that we compare the missing heartbeat count with HEARTBEAT_LIVENESS here:

if client.heartbeat < 0 and client.state != STATE_MISSING:
(instead of checking if it it below zero). Would that work? (that way we don't have to pass HEARTBEAT_LIVENESS around)

Do you think you could do that? We should probably rename the variable to .missed_heartbeats and HEARTBEAT_LIVENESS to something like MAX_MISSED_HEARTBEATS or something...

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.

2 participants