General question: Are we certain that the isLeaderFunc() return value is always updated before receiving the leadershipStateNotif notification?
What I’d like to confirm is that there’s no potential race condition between these two lines. For example, the notification might be sent because the DCA instance became the leader, but this information hasn’t yet propagated to the c.isLeaderFunc() output. This could result in skipping the execution of c.triggerReconciliation().

Would it make sense to add a log in the else case, stating: “Received a notification but not recognized as leader yet”?

The notification is never sent before the leadership status is updated fully and propagated to isLeader

Here is where notify is called. You can see that we synchronously call updateLeaderIdentity before we notify subscribers.

Finally, IsLeader has a very simple implementation. Once we updateLeaderIdentity, the change is instantly visible in isLeader func result.

Would it make sense to add a log in the else case, stating: “Received a notification but not recognized as leader yet”?

If it is not clear, the notification is not to tell the subscriber that the instance has become a leader. But rather to tell it that a change has occurred on the leadership state.

The subscriber is responsible to handle the logic based on the usecase, and isLeaderFunc is provided to determine the live state.

To add, I already added unit tests for the subscription feature which validates that the change will have already been propagated to isLeaderFunc when the notification is sent.

Q: How did this leadership election refactor address the possible blocking in the case of stuck subscribers. Do we still need to make the channel's buffer larger?

Check the implementation of the notify function.

If the buffer is already full, no new notifications are placed on the channel.

We only notify on the subscriber's channel if the channel is already empty, so we will never get stuck when writing to the channel.

basically this

Gotcha that makes sense. I'm trying to think of edge cases in leadership election (such as when leadership may be gained, lost, and then regained quickly) and how this might effect the overall state.

In such a case where

1. The Cluster Agent X is elected leader and sends a notification to it's subscribers via notify()
2. The subscriber does not get CPU time to process the notification.
3. The Cluster Agent X is then removed as leader and attempts to send a notification to it's subscribers via notify()

• However in this case, since the channel is full (buffer len = 1), it will not send a second notification.

4. The subscriber is finally scheduled on the CPU and only receives the first Leader Notification and doesn't know about the lost leadership notification.

The buffer length of 5 provided some resiliency in this type of situation I believe but also isn't perfect itself.

then regained quickly

The leadership lease duration is 60 seconds, so there is no such "quick" back-and-forth.

Regardless, it is still ok even in the case you explained:

and doesn't know about the lost leadership notification.

This is not quite accurate.
There no difference between the notification sent in case of acquisition of leadership and in case of loss of leaderhip.

In both cases, the notification is exactly the same, an empty struct struct{}{}. A notification sent on the channel is not telling the subscriber whether or not the current instance is the leader. It is only telling the subscriber that there has been changes to the leadership state of the current instance. The subscriber can then determine live leadership state via IsLeaderFunc which is returned also in the subscribe method.

In other words, these notifications are sorts of alerts to subscribers that tell them that they might need to check the new leadership state because there has been some changes. So, if the subscriber hasn't yet consumed the previous alert, it is pointless to send another identical alert. We can just keep the old alert and the subscriber will pick it up at some point in time when it has enough CPU.

This is made clear in the method comment.

Thanks for the explanation Adel about tracking changes.

So it seems in your implementation, if the leader cluster agent first sends a notification when it lost leadership, subscriber doesn't have time to read it, then sends another notification when it re-gains leadership, the subscriber may then get unblocked and process the notification but there has been no perceived change in leader. Whereas previously it would've been aware of the 2 changes in leadership that occurred via the buffer. I don't know if this slight change in behavior is relevant for consistency but it seems fine.

Overall it looks good to me if this isn't a concern.

Previously, the notification was sent only when the instance becomes a leader. No notification was being sent in case of leadership loss.

Therefore, in the scenario you described, in both implementations, only 1 notification is sent.

Perhaps what you wanted to say is that if the instance acquires leadership, loses it, then acquires leadership again, in the old implementation, this would've produced 2 notifications regardless of whether the subscriber consumed the first implementation or not. In the new implementation, this will ideally send at most 3 notifications assuming that the subscriber consumes each notification immediately. However, if the subscriber doesn't consume the first notification at all, the second and third notifications won't be sent. Then if after that the subscriber is able to consume the notification on the channel, it will then be able to check the IsLeader function.

It is important to note that the subscriber doesn't care about the history of changes in the leadership state. In practice this information is not useful in most cases. Subscribers are only interested in being notified when the state has changed. From the point of view of the subscriber, saying simultaneously that leadership state changed once or leadership state changed N times are essentially equivalent, because the subscriber doesn't care about the history of changes, but rather about the most recent state.

If at some point in the future we have a use case where the subscriber is interested in knowing the number of changes that occurred in the past, we can modify the code to support that. For now, this use case doesn't exist and is not expected to be there any time soon.

I hope this makes sense.

We should perhaps add a comment saying this code is racy, BUT there is now way to loose a state as the as if the reader reads after len(s) > 0 check, it's still going to use the isLeaderFunc and retrieve the last accurate state.