Don't unconditionally send consumer snapshot when becoming leader #4965
Conversation
Signed-off-by: Neil Twigg <neil@nats.io>
| @@ -4660,16 +4660,6 @@ func (js *jetStream) monitorConsumer(o *consumer, ca *consumerAssignment) { | |||
| js.setConsumerAssignmentRecovering(ca) | |||
| } | |||
|
|
|||
| // Synchronize everyone to our state. | |||
There was a problem hiding this comment.
Should we remove all together or adjust to make sure our upper state is correct?
It is there for a reason :)
There was a problem hiding this comment.
Do you remember what that reason was? I can't find anything written down as to why, the best I can find is from the commit message in cc77d66 which says:
Also sync other consumers when taking over as leader but no need to process snapshots when we are in fact the leader.
As far as I can tell everything should work without this because the Raft machinery would be keeping the ConsumerStore up-to-date on the replicas in the usual way anyway, whereas sending snapshots from the upper layer directly opens up a potential for a race between the committed Raft node state and the applied upper layer state, but I'm not sure if I'm missing something here.
There was a problem hiding this comment.
We were experiencing state skew, and I wanted us synched. But could have been from a prior bug that has since been corrected.
Feasibly a Raft node that is keeping up with its peers can become a leader regardless of whether or not the upper layer is caught up with its applies. Since the leader change notification to the upper state machine could interleave with the apply queue, then unconditionally sending a snapshot as a new append entry on consumer leader transitions could cause the rest of the group to revert to an earlier consumer state.
Signed-off-by: Neil Twigg neil@nats.io