[1.26 hotfix] limit fetch commit response size and increase request & response limit #18038
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
| .into_iter() | ||
| .take_while(|c| { | ||
| accumulated_size += c.serialized().len(); | ||
| accumulated_count += 1; |
There was a problem hiding this comment.
It feels like this accumulated_count is not necessary and we should limit the size of commits that are allowed to be written in store so that we are guaranteed to atleast have one commit in store that is < TARGET_RESPONSE_DATA_SIZE
There was a problem hiding this comment.
Actually not sure how you can cleanly limit this from the commit side, ignore the comment
There was a problem hiding this comment.
I don't think we can choose to not write some commits in store or sent them via commit sync.
consensus/config/src/parameters.rs
Outdated
| @@ -199,7 +199,7 @@ impl TonicParameters { | |||
| } | |||
|
|
|||
| fn default_message_size_limit() -> usize { | |||
| 8 << 20 | |||
| 32 << 20 | |||
There was a problem hiding this comment.
We have already seen 30MB in error responses from validators. Shall we go slightly over ex 35MB?
| @@ -29,6 +29,8 @@ use crate::{ | |||
| CommitIndex, Round, | |||
| }; | |||
|
|
|||
| const TARGET_RESPONSE_DATA_SIZE: usize = 3 * 1024 * 1024; | |||
There was a problem hiding this comment.
Isn't that too low? Especially with having on the other side a limit 30MB . I would recommend something around 10MB
There was a problem hiding this comment.
This is not a hard limit. It is a soft limit to make sure responses can be transmitted in time.
There was a problem hiding this comment.
hhm what do you mean by not a hard limit? It eventually can reduce the returned batch size, isn't it?
| @@ -321,7 +321,7 @@ impl<C: NetworkClient> CommitSyncer<C> { | |||
| start: CommitIndex, | |||
| end: CommitIndex, | |||
| ) -> ConsensusResult<(Vec<TrustedCommit>, Vec<VerifiedBlock>)> { | |||
| const FETCH_COMMITS_TIMEOUT: Duration = Duration::from_secs(10); | |||
| const FETCH_COMMITS_TIMEOUT: Duration = Duration::from_secs(30); | |||
There was a problem hiding this comment.
Shall we take a smaller step here and go to 20s?
| @@ -199,7 +199,7 @@ impl TonicParameters { | |||
| } | |||
|
|
|||
| fn default_message_size_limit() -> usize { | |||
| 8 << 20 | |||
| 2 << 30 | |||
There was a problem hiding this comment.
Let's also address the comment on line 187
|
Picked directly by @ebmifa. |
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.