Send notifications in batches #5054
Open
+287
−100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 1, 2025
Closed
Contributor
Author
ndr-ds
commented
Dec 1, 2025
ndr-ds
force-pushed
the
11-28-make_notification_and_cross_chain_queue_sizes_grow_with_network_size
branch
from
0119199 to
4a1d5ca
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
from
b1e7db4 to
106ed89
Compare
ndr-ds
force-pushed
the
11-28-make_notification_and_cross_chain_queue_sizes_grow_with_network_size
branch
from
4a1d5ca to
c627bad
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
from
106ed89 to
5c1a62d
Compare
ndr-ds
force-pushed
the
11-28-make_notification_and_cross_chain_queue_sizes_grow_with_network_size
branch
from
c627bad to
6c566c1
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
2 times, most recently
from
3a99dd2 to
8473288
Compare
ndr-ds
force-pushed
the
11-28-make_notification_and_cross_chain_queue_sizes_grow_with_network_size
branch
from
6c566c1 to
469db12
Compare
ndr-ds
changed the base branch from
11-28-make_notification_and_cross_chain_queue_sizes_grow_with_network_size
to
graphite-base/5054
ndr-ds
force-pushed
the
graphite-base/5054
branch
from
469db12 to
58b94ab
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
from
8473288 to
742eb97
Compare
ndr-ds
changed the base branch from
graphite-base/5054
to
11-28-several_metrics_changes
This was referenced Dec 11, 2025
This was referenced Dec 12, 2025
ndr-ds
force-pushed
the
11-28-several_metrics_changes
branch
from
58b94ab to
8b41b5a
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
from
742eb97 to
6e01efd
Compare
This was referenced Dec 15, 2025
Draft
ndr-ds
force-pushed
the
11-28-several_metrics_changes
branch
from
8b41b5a to
bb38573
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
2 times, most recently
from
3dbb400 to
514f4b7
Compare
ndr-ds
force-pushed
the
11-28-several_metrics_changes
branch
from
bb38573 to
26e0c2b
Compare
Contributor
Author
|
I can also try to keep |
ndr-ds
force-pushed
the
11-28-several_metrics_changes
branch
from
26e0c2b to
c0eced0
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
2 times, most recently
from
b618da9 to
dc6d19a
Compare
ndr-ds
force-pushed
the
11-28-several_metrics_changes
branch
from
c0eced0 to
73b4c94
Compare
ndr-ds
force-pushed
the
12-01-send_notifications_in_batches
branch
from
dc6d19a to
3876afe
Compare
Contributor
|
For the future (but also this PR): When you're making PRs that claim to improve performance I'd like to see some comparison/data for before vs after. It can be screenshots from Grafana, copy-paste from the terminal etc. |
afck
approved these changes
Jan 8, 2026
Contributor
afck
left a comment
afck
left a comment
There was a problem hiding this comment.
So this batches notifications when sending them server→proxy or server→exporter, but not proxy→client, right? (Would be helpful to clarify that in the description.)
I'd also like to see more data about how this improves performance. Approving assuming the improvement is significant.
| exporter_clients: Vec<NotifierServiceClient<Channel>>, | ||
| pending_notifications: Vec<Notification>, | ||
| futures: FuturesUnordered<BoxFuture<'static, ()>>, | ||
| tasks_in_flight: usize, |
Contributor
There was a problem hiding this comment.
Isn't this redundant? Isn't it always equal to self.futures.len()?
| } | ||
| Err(status) => { | ||
| // We assume errors when parsing are not critical and just log them. | ||
| tracing::warn!(error=?status, "received bad notification"); |
Contributor
There was a problem hiding this comment.
(Not in this PR, but: Why not call the variable error? Or log it as ?status?)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Sending notifications one at a time creates high RPC overhead. Batching notifications reduces overhead and improves
throughput.
Proposal
NotifyBatchRPC method for sending multiple notifications at onceBatchForwarderfor collecting and sending batched notificationsTest Plan
Release Plan