-
Notifications
You must be signed in to change notification settings - Fork 801
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
Broadcast VC requests in parallel and fix subscription error #6223
Broadcast VC requests in parallel and fix subscription error #6223
Conversation
Squashed commit of the following: commit 763a6ae Author: Michael Sproul <[email protected]> Date: Mon Aug 5 13:27:55 2024 +1000 Broadcast VC requests in parallel
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.
LGTM
This isn't working as well as expected (we still seem to have requests getting massively delayed), so I'm trying some improvements |
Squashed commit of the following: commit f0b4a0a Author: Michael Sproul <[email protected]> Date: Tue Aug 6 11:58:56 2024 +1000 Try some things commit e2f1875 Author: Michael Sproul <[email protected]> Date: Tue Aug 6 11:50:35 2024 +1000 Remove outdated comment commit 763a6ae Author: Michael Sproul <[email protected]> Date: Mon Aug 5 13:27:55 2024 +1000 Broadcast VC requests in parallel
Squashed commit of the following: commit d85687b Author: Michael Sproul <[email protected]> Date: Tue Aug 6 15:12:32 2024 +1000 Remove junk logging commit 1126795 Author: Michael Sproul <[email protected]> Date: Tue Aug 6 15:10:28 2024 +1000 Fix subscription error commit f0b4a0a Author: Michael Sproul <[email protected]> Date: Tue Aug 6 11:58:56 2024 +1000 Try some things commit e2f1875 Author: Michael Sproul <[email protected]> Date: Tue Aug 6 11:50:35 2024 +1000 Remove outdated comment commit 763a6ae Author: Michael Sproul <[email protected]> Date: Mon Aug 5 13:27:55 2024 +1000 Broadcast VC requests in parallel
I think I've fixed the bug in 1126795 Rolling out now |
Random fact:
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at a68f34a |
* Broadcast VC requests in parallel * Remove outdated comment * Try some things * Fix subscription error * Remove junk logging
* Broadcast VC requests in parallel * Remove outdated comment * Try some things * Fix subscription error * Remove junk logging
Issue Addressed
Closes:
broadcast
runs in series #5232Proposed Changes
Use
join_all
to run futures in parallel.This seemed a bit cleaner than using
JoinSet
, and is similarly efficient. Each loop iteration should be quick because.status().await
just checks an rwlock, it doesn't actually make any requests to the BN.This PR also fixes a bug whereby we wouldn't mark subscriptions as sent if any node failed. This meant that in the case of 1 offline node, subscriptions would continue getting spammed at the BN right up to the duty slot, which would result in the infamous warning:
This is fixed in 2 ways:
should_send_subscription_at
Along the way I also added a 500ms timeout for subscription requests, which previously had no timeout! I'm open to tweaking this timeout or putting it in another PR.
Additional Info
This improvement should help the performance of clusters of nodes when one of them goes AWOL due to an issue like: