VID task and exchange #1903
VID task and exchange #1903
Conversation
There was a problem hiding this comment.
Thank you! The amount of boilerplate required to add a new task is incredible. It's great to have a PR with all this in one place for future reference.
I have a bunch of minor comments, only some of which are actionable for this PR.
I'd like at least one other person on the consensus team to take a glance before we merge though.
| pub vid_exchange: Arc<VIDEx<TYPES, I>>, | ||
|
|
||
| /// The view and ID of the current vote collection task, if there is one. | ||
| pub vote_collector: Option<(TYPES::Time, usize, usize)>, |
There was a problem hiding this comment.
This comment is not VID-specific. Any action we take here could/should be replicated for all other task impls. Curious to hear opinions from other reviewers.
Do we need Option here?
| pub vote_collector: Option<(TYPES::Time, usize, usize)>, | |
| pub vote_collector: (TYPES::Time, usize, usize), |
It's only ever None when the task is first initialized in add_vid_task(). So now we need code everywhere that must accommodate the special case of node startup. There must be a better way to do this, and perhaps there's an easy, quick fix.
There was a problem hiding this comment.
It looks like we use it conditionally in a few places to shut down the task or start the accumulator if it hasn't already. Let me think of a more elegant solution
There was a problem hiding this comment.
Presumably the accumulator needs to be reset for each view, no? In that case, we should already have code to recognize when the accumulator needs to be reset for a new view. If so then presumably the same logic could be applied to the startup case.
There was a problem hiding this comment.
I'm thinking I can get another PR out for this one, and just get the VID stuff in without these changes in this one.
WDYT?
There was a problem hiding this comment.
agree. Also, this is low priority so don't sweat it. Let's get this merged.
There was a problem hiding this comment.
This a a pattern we need to rethink/address in the upcoming rework. I think the best thing to do is to spawn an accumulator task for every vote type and not have to worry about starting/killing tasks every from another task on a view by view basis.
Co-authored-by: Gus Gutoski <[email protected]>
Co-authored-by: Gus Gutoski <[email protected]>
Co-authored-by: Gus Gutoski <[email protected]>
Signed-off-by: Rob <[email protected]>
This is the MVP for separating out VID
Aims to close #1905