Change managing way for round queue #2151
Conversation
Signed-off-by: Konstantin Munichev <[email protected]>
| consensus::Round current_round = {i + 1, commit_round.reject_round}; | ||
| auto proposal = os->onRequestProposal({i + 1, commit_round.reject_round}); | ||
|
|
||
| if (current_round.block_round >= last_round.block_round) { |
There was a problem hiding this comment.
Seems, if else ladder could be refactored with inline function and used twice for this and EraseReject tests.
There was a problem hiding this comment.
Well, it's possible but these 2 code fragments are actually a bit different (using .block_round in the first one instead of .reject_round in the second) so I guess the resulting code will be too difficult to read.
Signed-off-by: Konstantin Munichev <[email protected]>
luckychess
requested review from
MBoldyrev and
muratovv
and removed request for
MBoldyrev
| @@ -334,14 +334,16 @@ TEST_F(OnDemandOsTest, PassMissingTransaction) { | |||
| auto proposal = os->onRequestProposal(target_round); | |||
|
|
|||
| // since we only sent one transaction, | |||
| // if the proposal is present, there is no need to check for that specific tx | |||
| // if the proposal is present, there is no need to check for that specific | |||
There was a problem hiding this comment.
Changes in this file are not really required by clang-format and can be reverted.
| proposal_map_.erase(round); | ||
| log_->info("tryErase: erased {}", round); | ||
| round_queue_.pop(); | ||
| while (proposal_map_.size() > number_of_proposals_) { |
There was a problem hiding this comment.
I would rather use the previous logic, where this loop operated only on subset of proposal_map_ with map.round < packNextProposals.round.
I would suggest to first it = proposal_map_.find(round), and then operate on range(proposal_map_.begin(), it).
There was a problem hiding this comment.
this makes sense. It could be also done by passing current round to tryErase and adding condition
| while (proposal_map_.size() > number_of_proposals_) { | |
| while (proposal_map_.size() > number_of_proposals_ and proposal_map_.begin()->first < current_round) { |
There was a problem hiding this comment.
I think that the first condition should only consider the subset, e.g. size(range(proposal_map_.begin(), it)) > number_of_proposals_ instead of proposal_map_.size() > number_of_proposals_.
Signed-off-by: Konstantin Munichev <[email protected]>
Signed-off-by: Konstantin Munichev <[email protected]>
| auto proposal_range_size = boost::size( | ||
| boost::make_iterator_range(proposal_map_.begin(), current_proposal)); | ||
|
|
||
| while (proposal_range_size > number_of_proposals_ |
There was a problem hiding this comment.
quite a strange condition, because proposal_range_size is unchanged in the loop. maybe
| while (proposal_range_size > number_of_proposals_ | |
| while (proposal_range_size-- > number_of_proposals_ |
| boost::make_iterator_range(proposal_map_.begin(), current_proposal)); | ||
|
|
||
| while (proposal_range_size > number_of_proposals_ | ||
| and proposal_map_.begin()->first < current_round) { |
There was a problem hiding this comment.
and if I get the code right, this can be converted into an assertion
Signed-off-by: Konstantin Munichev <[email protected]>
Signed-off-by: Konstantin Munichev <[email protected]>
Signed-off-by: Konstantin Munichev <[email protected]>
Description of the Change
Pull request fixes a race with proposal retrieval
Benefits
No race ^^
Possible Drawbacks
Tests code could be better for erase case.