Combine payload update and setting healthy status. #107
Conversation
| @@ -162,44 +158,23 @@ impl Gate { | |||
| } | |||
| } | |||
|
|
|||
| /// Updates the data set of the unit. | |||
| /// Updates the unit. | |||
There was a problem hiding this comment.
I find this one line summary confusing & misleading. What does it mean to "update" a unit? Units produce updates, they are not themselves updated by this function, right? Rather the downstreams are updated by the unit. If renaming this why not rename it to send_update() which more clearly says what it does.
There was a problem hiding this comment.
The units themselves now keep their state (to facilitate late arriving new links), so in my mental model, they are indeed updated by this method and just happen to also produce events sent to connected links when being updated.
src/comms.rs
Outdated
| @@ -286,7 +261,7 @@ impl GateAgent { | |||
| #[derive(Debug, Default)] | |||
| pub struct GateMetrics { | |||
| /// The current unit status. | |||
| status: AtomicCell<UnitStatus>, | |||
| status: AtomicCell<UnitHealth>, | |||
There was a problem hiding this comment.
Should the field also be renamed to 'health' instead of 'status'? Especially as health is a subfield of UnitStatus.
| /// | ||
| /// This may be `None` if the unit status indicates that there hasn’t | ||
| /// been an update yet. | ||
| fn to_update(&self) -> Option<UnitUpdate> { |
There was a problem hiding this comment.
I misread this thinking because of the to in the name that it would take Self (I guess I was wrongly thinking of into because they are similar). It's more like a mapping getter - what would you think about as_update() as a name instead?
There was a problem hiding this comment.
The standard naming convention for converting methods is that into_ takes Self, to_ takes a &Self, and as_ takes a &Self but returns a &T, so, to_update is the right name following that convention.
src/units/combine.rs
Outdated
| u2.status(Healthy).await; | ||
| assert_eq!(t.recv().await, Err(Healthy)); | ||
| assert_eq!(t.recv().await, Ok(testrig::update(&[1]))); | ||
| // Set one unit to healthy, check that we get an update. |
There was a problem hiding this comment.
| // Set one unit to healthy, check that we get an update. | |
| // Set one unit to healthy by sending an update, which should be received by the target. |
There was a problem hiding this comment.
Rephrased it in a slightly different way.
| u1.send_stalled().await; | ||
| assert_eq!(t.recv_payload().await.unwrap(), testrig::update(&[2])); | ||
|
|
||
| // Now stall the second one, too, and watch us stall. |
There was a problem hiding this comment.
| // Now stall the second one, too, and watch us stall. | |
| // Now stall the second one too, and watch us stall. |
There was a problem hiding this comment.
Not to get into a grammar discussion, but I have learned way back that there always is a comma before a too.
| component: Component, | ||
| target: Source, | ||
| mut notify: NotifySender | ||
| ) -> Result<(), ExitError> { |
There was a problem hiding this comment.
How does this code ever break out of the loop or return anything?
There was a problem hiding this comment.
It doesn’t. This never returns. Maybe it should, but perhaps that is a separate PR?
This PR revises how updates are dispatched. Instead of having separate status and payload updates, these are now merged into one with a payload update implying a status change to healthy. This should avoid the potential for strange races between setting the status back to healthy and updating the data.
A resulting limitation is that if a unit wants to change from stalled to healthy it has to submit a payload update, but in practice this health status change is pretty much always triggered by a new payload update becoming available, so this should be fine.
This is a breaking change.