Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Temporarily increase notifications buffer size #5644

Merged
merged 3 commits into from
Apr 15, 2020
Merged

Temporarily increase notifications buffer size #5644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ee6fa1
Temporarily increase notifications buffer size
tomaka Apr 15, 2020
c6c93b6
Add a 511.0 bucket
tomaka Apr 15, 2020
c8d88f8
Merge remote-tracking branch 'upstream/master' into notifs-queues-sizes
tomaka Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/network/src/protocol/generic_proto/upgrade/notifications.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use unsigned_varint::codec::UviBytes;
/// Maximum allowed size of the two handshake messages, in bytes.
const MAX_HANDSHAKE_SIZE: usize = 1024;
/// Maximum number of buffered messages before we refuse to accept more.
const MAX_PENDING_MESSAGES: usize = 256;
const MAX_PENDING_MESSAGES: usize = 512;

/// Upgrade that accepts a substream, sends back a status message, then becomes a unidirectional
/// stream of messages.
Expand Down
2 changes: 1 addition & 1 deletion client/network/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ impl Metrics {
"sub_libp2p_notifications_queues_size",
"Total size of all the notification queues"
),
buckets: vec![0.0, 1.0, 2.0, 4.0, 8.0, 16.0, 32.0, 64.0, 128.0, 256.0],
buckets: vec![0.0, 1.0, 2.0, 4.0, 8.0, 16.0, 32.0, 64.0, 128.0, 256.0, 511.0, 512.0],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buckets: vec![0.0, 1.0, 2.0, 4.0, 8.0, 16.0, 32.0, 64.0, 128.0, 256.0, 511.0, 512.0],
buckets: vec![0.0, 1.0, 2.0, 4.0, 8.0, 16.0, 32.0, 64.0, 128.0, 256.0, 512.0],

Why 511?

Copy link
Contributor

Choose a reason for hiding this comment

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

We start dropping messages above 512 leaving the length at 512.

Histogram buckets are upper bounds. Thus in the case where we don't have the 511 bucket and the histogram shows 512 we don't know whether the buffer length is

  • between 256 and 512 and thus not dropping packets, or

  • at 512 and thus dropping packets.

Introducing bucket 511 into the picture the Histogram will show 511 or lower if the buffer is not dropping packets and 512 if it is full or dropping packets.

Does that make sense @bkchr?

Copy link
Member

Choose a reason for hiding this comment

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

Not really, but if that is intentionally, okay :)

},
&["protocol"]
)?, registry)?,
Expand Down