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

Introduce async budget in transaction manager #6295

Merged
merged 19 commits into from
Feb 1, 2024
Merged

Conversation

emhane
Copy link
Member

@emhane emhane commented Jan 30, 2024

closes #6290

Copy link
Collaborator

@mattsse mattsse left a 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

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

nits

emhane and others added 3 commits February 1, 2024 00:06
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

few more nits

Copy link
Collaborator

@mattsse mattsse left a 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

@emhane
Copy link
Member Author

emhane commented Feb 1, 2024

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

@mattsse
Copy link
Collaborator

mattsse commented Feb 1, 2024

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();
Copy link
Member Author

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mattsse ? last remaining comment

@emhane emhane force-pushed the emhane/budget-tx-manager branch from 43147dd to a1fd6e9 Compare February 1, 2024 15:26
Copy link
Collaborator

@mattsse mattsse left a 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

@mattsse mattsse added this pull request to the merge queue Feb 1, 2024
Merged via the queue into main with commit cac6a52 Feb 1, 2024
28 checks passed
@mattsse mattsse deleted the emhane/budget-tx-manager branch February 1, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async budget in TransactionManager
4 participants