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

Notify telemetry only every second about the tx pool status #6605

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Nov 21, 2024

Before this was done for every imported transaction. When a lot of transactions got imported, the import notification channel was filled. The underlying problem was that the status call is read locking the validated_pool which will be write locked by the internal submitting logic. Thus, the submitting and status reading was interferring which each other.

Before this was done for every imported transaction. When a lot of transactions got imported,
the import notification channel was filled. The underlying problem was that the `status` call
is read locking the `validated_pool` which will be write locked by the internal submitting logic.
Thus, the submitting and status reading was interferring which each other.
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Nov 21, 2024
@michalkucharczyk
Copy link
Contributor

for the record: #6600 (comment)

@bkchr
Copy link
Member Author

bkchr commented Nov 22, 2024

/cmd prdoc --audience node_operator --bump patch

_ = timer => {
timer = futures_timer::Delay::new(TELEMETRY_INTERVAL).fuse();

if !tx_imported {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we could avoid needless wakeups, by only re-initializing the timer in case a new notification came in. E.g. replace above tx_imported = true; with:

if !tx_imported {
  tx_imported = true;
  timer = futures_timer::Delay::new(TELEMETRY_INTERVAL).fuse();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for deleting my previous comments here - I will reformulate them into a question.

future is not notified into import_notification_stream, also ready can be decreased by putting txs into the blocks.

Not sure if there is other place in transaction pool that updates this metric?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants