Do heartbeat on clients #9798
Merged
Do heartbeat on clients #9798
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CodSpeed Performance ReportMerging #9798 will improve performances by 10.2%Comparing Summary
Benchmarks breakdown
|
|
Tested with bigpoly on LSF (400 reals, 200 max running) and all was successfully reconnected.
|
xjules
force-pushed
the
fix_monitor
branch
2 times, most recently
from
887d748 to
bcd8563
Compare
jonathan-eq
reviewed
Jan 20, 2025
jonathan-eq
reviewed
Jan 20, 2025
| if ( | ||
| last_ping_time | ||
| and (asyncio.get_running_loop().time() - last_ping_time) | ||
| > 2 * PING_TIMEOUT |
There was a problem hiding this comment.
I guess the 2* here is for the ping to be received and returned, but do we need it if we are not returning the ping? Maybe in our case it should be added in the PING_TIMEOUT directly?
There was a problem hiding this comment.
This 2 * PING_TIMEOUT is to account for a potential delay, which can happen if the ee is very busy.
jonathan-eq
reviewed
Jan 20, 2025
jonathan-eq
reviewed
Jan 20, 2025
| @@ -93,11 +95,23 @@ async def process_message(self, msg: str) -> None: | |||
| raise NotImplementedError("Only monitor can receive messages!") | |||
|
|
|||
| async def _receiver(self) -> None: | |||
| last_beat_time: float | None = None | |||
There was a problem hiding this comment.
Maybe last_heartbeat_time?
jonathan-eq
requested changes
Jan 21, 2025
jonathan-eq
reviewed
Jan 21, 2025
| @@ -342,6 +360,7 @@ async def _start_running(self) -> None: | |||
| raise ValueError("no config for evaluator") | |||
| self._ee_tasks = [ | |||
| asyncio.create_task(self._server(), name="server_task"), | |||
| asyncio.create_task(self._do_heartbeat_clients(), name="beat_task"), | |||
There was a problem hiding this comment.
Suggested change
| asyncio.create_task(self._do_heartbeat_clients(), name="beat_task"), | |
| asyncio.create_task(self._do_heartbeat_clients(), name="heartbeat_task"), |
That has been changed, just the hearbeat_task not yet. |
sondreso
approved these changes
Jan 21, 2025
| @@ -51,7 +57,7 @@ def __init__(self, ensemble: Ensemble, config: EvaluatorServerConfig): | |||
| self._ensemble: Ensemble = ensemble | |||
|
|
|||
| self._events: asyncio.Queue[Event] = asyncio.Queue() | |||
| self._events_to_send: asyncio.Queue[Event] = asyncio.Queue() | |||
| self._events_to_send: asyncio.Queue[Event | bytes] = asyncio.Queue() | |||
There was a problem hiding this comment.
Could we be more specific here?
xjules
force-pushed
the
fix_monitor
branch
5 times, most recently
from
d2afc2a to
8589c06
Compare
jonathan-eq
reviewed
Jan 23, 2025
| @@ -45,13 +51,17 @@ | |||
| EVENT_HANDLER = Callable[[list[Event]], Awaitable[None]] | |||
|
|
|||
|
|
|||
| class HeartbeatEvent(Enum): | |||
| event = b"BEAT" | |||
There was a problem hiding this comment.
Rather than hardcoding b"BEAT", can we import it from client?
jonathan-eq
reviewed
Jan 23, 2025
Heartbeat task sends HEARTBEAT to all the clients (ie. Monitor) at client.HEARTBEAT_TIMEOUT intervals. Clients do not reply, just process the message. If client detects longer delay between two heartbeats, the client will send CONNECT to evaluator in addition; ie. getting the connection re-established after a break. This is to simulate re-connection. Each CONNECT_MSG will then trigger sending FullSnapshot from the ensemble evaluator. Initially HEARTBEAT_TIMEOUT is set to 5 seconds while Monitor accepts 10 seconds at max as a delay. Additionally, initial connection will now undergo same amount of retries as standard messages.
sondreso
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Resolves #9782
Approach
Heartbeat task sends HEARTBEAT to all the clients (ie. Monitor) at client.HEARTBEAT_TIMEOUT intervals.
Clients do not reply, just process the message. If client detects a longer delay between two heartbeats, the client will send CONNECT to evaluator in addition; ie. getting the connection re-established after a break. This is to simulate re-connection. Each CONNECT_MSG will then trigger sending FullSnapshot from the ensemble evaluator.
Initially HEARTBEAT_TIMEOUT is set to 5 seconds while Monitor accepts 10 seconds at max as a delay. Additionally, initial connection will now undergo same amount of retries as standard messages.
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'pytest tests/ert/unit_tests -n auto -m "not integration_test"')When applicable