Provide an applicative network heartbeat#15
Merged
18 commits merged intoJun 10, 2021
Merged
Conversation
This probably makes the whole HeartbeatMessage / HydraMessage dichotomy redundant but I keep it for now for the sake of illustrating map/contramap transformation of messages.
As Heartbeat logic has moved out of Network layer, ensuring messages are properly broadcasted requires actively retrying until they are seen on all nodes, as startup time might lead to messages being dropped.
ghost
requested review from
ch1bo
requested changes
Jun 9, 2021
| ReqSn -> toCBOR ("ReqSn" :: Text) | ||
| AckSn -> toCBOR ("AckSn" :: Text) | ||
| ConfSn -> toCBOR ("ConfSn" :: Text) | ||
| Ping pty -> toCBOR ("ConfSn" :: Text) <> toCBOR pty |
Member
There was a problem hiding this comment.
Wrong identifier!
Suggested change
| Ping pty -> toCBOR ("ConfSn" :: Text) <> toCBOR pty | |
| Ping pty -> toCBOR ("Ping" :: Text) <> toCBOR pty |
|
|
||
| broadcast hn3 requestTx | ||
| failAfter 1 $ takeMVar node1received `shouldReturn` MessageReceived requestTx | ||
| failAfter 1 $ takeMVar node2received `shouldReturn` MessageReceived requestTx |
Member
There was a problem hiding this comment.
Would this not pass anymore? The new assertions do simultaneously broadcast the same message from different senders, right?
Author
There was a problem hiding this comment.
Make each requestTx different for each node so that we have a narrower shotgun...
|
|
||
| waitForOtherNodesConnected :: [Int] -> HydraNode -> IO () | ||
| waitForOtherNodesConnected allNodeIds n@HydraNode{hydraNodeId} = | ||
| mapM_ (\otherNode -> waitForResponse 10 [n] $ "NodeConnectedToNetwork " <> show otherNode) $ filter (/= hydraNodeId) allNodeIds |
Member
There was a problem hiding this comment.
Maybe rename the output to NodeConnectedToPeer?
KtorZ
reviewed
Jun 10, 2021
| forever $ do | ||
| threadDelay 0.5 | ||
| st <- atomically $ readTVar heartbeatState | ||
| when (st == SendHeartbeat) $ broadcast (Ping me) |
Collaborator
There was a problem hiding this comment.
Couldn't this exit instead when the heartbeat is stopped? Or do we expect the heartbeat to be restarted?
KtorZ
reviewed
Jun 10, 2021
|
|
||
| metrics <- getMetrics n1 | ||
| metrics `shouldSatisfy` ("hydra_head_events 3" `BS.isInfixOf`) | ||
| metrics `shouldSatisfy` ("hydra_head_events 5" `BS.isInfixOf`) |
Collaborator
There was a problem hiding this comment.
🤔 ? Pings are now included?
KtorZ
approved these changes
Jun 10, 2021
This pull request was closed.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Remove connectivity check from concrete
HydraNetworkimplementations and implement a genericHeartbeatwrapper that independently sends heartbeats/pings across the network for some time.This also illustrates the "decorator" pattern as exposed in https://github.com/input-output-hk/hydra-poc/blob/master/docs/adr/0007-with-pattern-component-interfaces.md
Next steps: