-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Introduce async budget in transaction manager #6295
Conversation
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 forgot that we don't have a main loop where a single budget would be appropriate
perhaps we should break instead
and also take another look at the function as a whole
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.
nits
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
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.
few more nits
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.
this is now an entirely new logic
I prefer the previous one and then revisit this separately, because this was already reviewed.
so we can make progress with this here
made some changes. now every stream can be prioritised differently. though I advice against it, if you still want, I can disable the budget for tx propagation @mattsse |
can we please roll this back to 35e375e first with the nits applied and then revisit separately? |
|
||
// try drain buffered transactions | ||
this.request_buffered_hashes(); | ||
this.update_request_metrics(); |
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.
do we want to update this metric before and after poll the tx fetcher for events? this is how it is on main
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.
@mattsse ? last remaining comment
43147dd
to
a1fd6e9
Compare
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.
we should be able to improve this further, something like your additional design but I'd like to test that a bit more first so we get a better understanding what the actual bottleneck here is, most likely incoming network events
we can track this in an issue
closes #6290