Skip to content
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

Revert #1409 partially #1603

Merged
merged 8 commits into from
Sep 18, 2023
Merged

Conversation

vstakhov
Copy link
Contributor

Futures channels that are used by default has a side effect of Sender::Clone that efficiently increases the capacity of the bounded channel by one. This PR fixes the undesired backpressure removal that was caused by the #1409. This issue has been discovered by @sandreim during Versi testing and needs to be treated as critical that should not be included in any release without this reversion.

This PR reverts the original behaviour that can be slightly improved without cloning the sender.

@vstakhov vstakhov added A0-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix T0-node This PR/Issue is related to the topic “node”. I2-bug The node fails to follow expected behavior. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Sep 17, 2023
@vstakhov vstakhov marked this pull request as ready for review September 18, 2023 06:28
@vstakhov vstakhov requested a review from sandreim September 18, 2023 07:26
polkadot/node/network/bridge/src/rx/mod.rs Outdated Show resolved Hide resolved

if delayed_messages_count > 0 {
// Here we wait for all the delayed messages to be sent.
let _timer = metrics.time_delayed_rx_events(); // Dropped after `await` is completed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This metric is useful, why not keep it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR just reverts the "bad things", so I've decided to leave it as simple and quick as possible. I'm working on a better version in the meantime on top of this one.

@vstakhov
Copy link
Contributor Author

bot merge

@command-bot
Copy link

command-bot bot commented Sep 18, 2023

@vstakhov bot merge and bot rebase are not supported anymore. Please use native Github "Auto-Merge" and "Update Branch" buttons instead.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I2-bug The node fails to follow expected behavior. T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants