Only broadcast HostRemovedFromCluster when a host is part of a cluster
#1611
Conversation
| @@ -275,6 +275,17 @@ defmodule Trento.HostProjector do | |||
| end | |||
| end | |||
|
|
|||
| def after_update( | |||
| %HostRemovedFromCluster{host_id: id, cluster_id: cluster_id}, | |||
There was a problem hiding this comment.
We don't need the cluster_id here.
This field is used to know from which cluster to remove the host.
Here we need to broadcast cluster: nil only if cluster_id coming in the event matched with the current cluster id stored in the db.
You could do that pattern matching the 3rd argument:
def after_update(
%HostRemovedFromCluster{host_id: id},
_,
%{host: %HostReadModel{cluster_id: nil}}
) do
With this clause, you would only run the broadcast if there was a change in the multi.
Anyway, we should have the tests to confirm it hehe
jamie-suse
force-pushed
the
broadcast-host-removed-from-cluster
branch
from
ff9076a to
d615d40
Compare
| %{host: %Trento.HostReadModel{cluster_id: nil}} | ||
| ) do | ||
| TrentoWeb.Endpoint.broadcast("monitoring:hosts", "host_details_updated", %{ | ||
| id: host_id |
There was a problem hiding this comment.
Is this the correct payload to broadcast? cluster_id will always be nil because of the pattern-matching, is it useful to transmit this?
There was a problem hiding this comment.
No, we need to explicitily send cluster_id: nil, because this is the value we want to store in the frontend.
If we don't send it, actually we are not sending any information.
This message is used to merge the already stored host data in redux with the new one, where we want to set the cluster as nil
|
|
||
| refute_broadcast "host_details_updated", | ||
| %{id: ^host_id}, | ||
| 1000 |
There was a problem hiding this comment.
I'm just a bit worried about this 1000 value.
In the refute this blocks the test until the timeout is expired, so it will be waiting for 1 second to have a green test.
Maybe we should use the default 100 value?
@CDimonaco @fabriziosestito Any suggestion on this?
There was a problem hiding this comment.
I think the 1000 value was set by me when I started doing broadcast test, this was set by testing manually different values, if the default timeout is sufficient we can switch to that
There was a problem hiding this comment.
I mean, in assert_broadcast is fine, because it will just wait until the message is sent. And it will be a long test only on failure.
The refute will always wait for the time, even in passing tests.
That's the difference
There was a problem hiding this comment.
I reduced this timeout to 100 mS 👍
There was a problem hiding this comment.
Just a least request XD
Could you test if putting this same refute_broadcast with 100ms in the test above fails? Of course, just temporarily, in your local env.
It should fail. I want to be sure that the 100ms makes sense
There was a problem hiding this comment.
Just did on my local machine and refute_broadcast can catch messages using a timeout as low as 1 mS 😵 I guess we may see different performance in the CI environment though.
|
@jamie-suse Green light from my side! |
Description
Relates to #1474
This change will broadcast
HostRemovedFromClusteronly when a given host is part of a cluster.How was this tested?
Added conditions in unit tests to assert for the existence/nonexistence of the broadcast messages when
HostRemovedFromClusteris called