[QuorumStore] Fix batch requester #7190
Merged
Conversation
This file contains 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
danielxiangzl
changed the title
bchocho
reviewed
Mar 16, 2023
| } | ||
| } else { | ||
| // retry_limit exceeds, fail to get batch from peers | ||
| break; |
There was a problem hiding this comment.
Hm, so if we break here, the last set of peers gets only 1 second to receive RPC responses? Instead of a break immediately, is the behavior we want to wait until the futures are drained?
There was a problem hiding this comment.
Good point! Resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…into daniel-qs-counter
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bchocho
reviewed
Mar 16, 2023
| for peer in request_peers { | ||
| futures.push(network_sender.request_batch(request.clone(), peer, rpc_timeout)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Would this work?
else if (futures.len() == 0) {
break;
}
Having another num_futures just feels a bit brittle.
zekun000
reviewed
Mar 16, 2023
| let network_sender = self.network_sender.clone(); | ||
| let request_num_peers = self.request_num_peers; | ||
| let my_peer_id = self.my_peer_id; | ||
| let epoch = self.epoch; | ||
| let timeout = Duration::from_millis(self.request_timeout_ms as u64); | ||
| let retry_timeout = Duration::from_millis(self.retry_timeout_ms as u64); |
There was a problem hiding this comment.
the name is confusing to me, we probably should call it "retry_interval" or something similar
| num_futures -= 1; | ||
| if let Ok(batch) = response { | ||
| counters::RECEIVED_BATCH_RESPONSE_COUNT.inc(); | ||
| if batch.verify().is_ok() { |
There was a problem hiding this comment.
I think you need a rebase
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
igor-aptos
approved these changes
Mar 17, 2023
| if self.num_retries == 0 { | ||
| let mut rng = rand::thread_rng(); | ||
| // make sure nodes request from the different set of nodes | ||
| self.next_index = rng.gen::<usize>() % self.signers.len(); |
There was a problem hiding this comment.
Are peers shuffled or sorted? If sorted, can it be a problem that we are using a consecutive range?
There was a problem hiding this comment.
I think it's shuffled
zekun000
approved these changes
Mar 17, 2023
| if self.num_retries == 0 { | ||
| let mut rng = rand::thread_rng(); | ||
| // make sure nodes request from the different set of nodes | ||
| self.next_index = rng.gen::<usize>() % self.signers.len(); |
There was a problem hiding this comment.
I think it's shuffled
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
bchocho
pushed a commit
that referenced
this pull request
Mar 17, 2023
* fix batch request counter, add retry limit to config * new batch requester
bchocho
added a commit
that referenced
this pull request
Mar 17, 2023
* fix batch request counter, add retry limit to config * new batch requester Co-authored-by: danielx <[email protected]>
Merged
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.
Description
Fix the batch requester to send batch requests to peers every retry_timeout interval (via RPC request of rpc_timeout) up to retry_limit times. Fix related counters, and move related parameters to the QS config file.
Test Plan