-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't send ResponseMessage::NotFound
right away, wait for txns
#3231
Conversation
crates/task-impls/src/response.rs
Outdated
if consensus | ||
.calculate_and_update_vid(view, Arc::clone(&self.quorum), &self.private_key) | ||
.await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I missed this in a previous pr but I think we probably want to move the calculation of vid shares back outside of consensus. This forces a write lock to be held to calculate VID which will basically block everything.
I think just the update VID should need the lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. My mistake I extended the write lock.
I decided to keep the function in Consensus
but as a associated function which takes LockedConsensusState
and locks it accordingly, either read or write as needed. I wanted to keep it in a separate function because exactly the same operations are used in both Response and DA tasks.
Side effect of this change is that now get_or_calc_vid_share
locks Consensus
in a more fine-grained manner, i.e. locks more often but for one operation only. I don't think it's a problem. Afaict locking is not considered as a "heavy" operation.
Closes #3046
This PR:
ResponseMessage::NotFound
immediatelyThis PR does not:
This change delays the response to the requester which blocks the requester from moving to the next possible responder.
This might be viewed as undesirable behaviour depending on the scenario.
Key places to review:
get_or_calc_vid_share
methodAll tests should pass.