narwhal icon indicating copy to clipboard operation
narwhal copied to clipboard

Replace consensus number notification Atomic with async watch

Open gdanezis opened this issue 3 years ago • 1 comments

Refactor the cleanup code to use a watch that notifies other components of the consensus number change, instead of actively polling a shared Atomic u64 at every iteration of each select loop.

gdanezis avatar Jul 27 '22 05:07 gdanezis

I’ll be back fully back tomorrow and will be able to give a complete review. In the meantime, have you considered re-using the “reconfiguration” watch channel to propagate the consensus round? We may simply add another enum variant to this channel? The sender is held by the state handler and virtually every other task has a receiver.

asonnino avatar Jul 27 '22 13:07 asonnino

I’ll be back fully back tomorrow and will be able to give a complete review. In the meantime, have you considered re-using the “reconfiguration” watch channel to propagate the consensus round? We may simply add another enum variant to this channel? The sender is held by the state handler and virtually every other task has a receiver.

@asonnino really good idea. The only thing that holds me a bit back are two things:

  1. Giving too much responsibility to the reconfiguration channel and effectively couple our selfs - now too many messages will go via the same topic
  2. The nature of the watch channel is to only keep the last sent value. That being said I am not sure of any edge cases we could observe here as all those are important messages.

But I agree it probably saves us monitoring another channel. Let's think that again separately?

akichidis avatar Aug 01 '22 09:08 akichidis