c/node_status_backend: fix handling members notification #11468
Conversation
If the members table update is coming from a controller snapshot, and the node is already being decommissioned, there won't be a notification with status "added", only with status "draining". Node discovery code should handle this case as well.
| @@ -93,7 +93,9 @@ ss::future<> node_status_backend::drain_notifications_queue() { | |||
|
|
|||
| ss::future<> node_status_backend::handle_members_updated_notification( | |||
| model::node_id node_id, model::membership_state state) { | |||
| if (state == model::membership_state::active) { | |||
| switch (state) { | |||
| case model::membership_state::active: | |||
There was a problem hiding this comment.
this looks fine but I thought you mentioned something about issuing notifications differently when loading from a snapshot (thats probably going to be a little more complicated than this patch) but just wanted to check..
There was a problem hiding this comment.
There is a bit of a subtlety with semantics of these notifications - previously they looked like "here is a node id, something happened to it", which is bit lacking, but robust. Recently, state information was added to them so the clients of these notifications may think that they would allow tracing all the history of changes - which is obviously impossible with snapshots. So the clients should deal with the fact that the won't see some state changes.
But I guess generating at least an "added" notification won't hurt - we can make it more robust both on the client and the generation side.
There was a problem hiding this comment.
So I thought for a while and decided to keep the current members_table behavior. This is essentially a difference between edge-triggered and level-triggered events and I think it is better to stick to one or another (level-triggered in this case). Added a clarifying comment to members_table.
If the members table update is coming from a controller snapshot, and the node is already being decommissioned, there won't be a notification with status "added", only with status "draining". Node discovery code should handle this case as well.
(partially) fixes #9052
Backports Required
Release Notes