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

approval-distribution: batched approval/assignment sending #6401

Merged
merged 12 commits into from
Dec 13, 2022
+222 −31
Merged

approval-distribution: batched approval/assignment sending #6401

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
32d2ab8
Imple batched send
sandreim Dec 6, 2022
a938c47
Add batch tests
sandreim Dec 6, 2022
0e81e4f
pub MAX_NOTIFICATION_SIZE
sandreim Dec 6, 2022
27668d9
spell check
sandreim Dec 6, 2022
b26f42d
spellcheck ...
sandreim Dec 6, 2022
da40262
Fix comment
sandreim Dec 6, 2022
1840ae2
o.O
sandreim Dec 6, 2022
8fd2ae6
2 constants
sandreim Dec 12, 2022
4525c1e
Ensure batch size is at least 1 element
sandreim Dec 12, 2022
ff23cb9
feedback impl
sandreim Dec 12, 2022
e5782dc
Merge branch 'master' of github.com:paritytech/polkadot into sandreim…
sandreim Dec 12, 2022
668436b
Merge branch 'master' of github.com:paritytech/polkadot into sandreim…
sandreim Dec 13, 2022
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
Prev Previous commit
Next Next commit
spell check 
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
  • Loading branch information
@sandreim
sandreim committed Dec 6, 2022
commit 27668d97838010f1cc1b7ffbca1b34867f5c2b01
2 changes: 1 addition & 1 deletion node/network/approval-distribution/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,7 +1742,7 @@ pub const MAX_BATCH_SIZE: usize = MAX_NOTIFICATION_SIZE as usize /
std::mem::size_of::<(IndirectAssignmentCert, CandidateIndex)>() /
3;
Copy link
Member

Choose a reason for hiding this comment

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

Would be safer if we guaranteed via min that it is at least as large as a single message.

Copy link
Member

Choose a reason for hiding this comment

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

Why is the size the same for approvals and assignments. I would assume that assignments are larger, so you just picked the larger one. ... Still just having two constants here would be hardly more work, but more correct and a bit more robust against future changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. Initially I had 2 constants, but Idecided that it would be simpler with one, without compromising on anything else. I'll change it back to 2 constants and also provide the said guarantee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


/// Send assignments while honouring the `max_notification_size` of the protocol.
/// Send assignments while honoring the `max_notification_size` of the protocol.
///
/// Spliting the messages into multiple notifications allows more granular processing of messages
/// at the destination, such that the subsystem doesn't get stuck for long processing a batch
Expand Down