Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Resending votes in consensus #2200

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

muratovv
Copy link
Contributor

Description of the Change

The PR introduce changes for resending votes. The feature is required due to we want to simulate the asynchronous environment among YAC processors.

Design

  • Fix voting interface of consensus: now it requires value types for propagation.
  • Add new interface for proto transport level with has feedback about the status of sending
  • Add wrapper which can resend votes if it is required

Possible Drawbacks

  • Lack of new proto transport test with different statuses.
  • Probably there are missing two things: the decline of existed calls and attempt limitation.

Signed-off-by: Fedor Muratov muratovfyodor@yandex.ru

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>
Copy link
Contributor

@MBoldyrev MBoldyrev left a comment

Choose a reason for hiding this comment

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

Please re-enable the test or provide a reason of its disabling. Also, there might be a problem as MST regression, as there is another MST test showing spontaneous failures - it is worth further investigation.

@@ -39,7 +39,7 @@ namespace iroha {
/**
* Provide current leader peer
*/
const shared_model::interface::Peer &currentLeader();
const std::shared_ptr<shared_model::interface::Peer> &currentLeader();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make

Suggested change
const std::shared_ptr<shared_model::interface::Peer> &currentLeader();
std::shared_ptr<shared_model::interface::Peer> currentLeader();

as returning a reference to a local object is scary, and then

Suggested change
const std::shared_ptr<shared_model::interface::Peer> &currentLeader();
std::shared_ptr<const shared_model::interface::Peer> currentLeader();

as we do not want anyone to change the object (no matter it is an interface)

StatusCode::UNIMPLEMENTED,
StatusCode::UNAVAILABLE,
StatusCode::DATA_LOSS};
return std::any_of(codes.begin(), codes.end(), [code](auto val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return std::any_of(codes.begin(), codes.end(), [code](auto val) {
return std::find(codes.begin(), codes.end(), code) != codes.end();

or, if using set,

Suggested change
return std::any_of(codes.begin(), codes.end(), [code](auto val) {
return codes.find(code) != codes.end();

which is faster


auto is_troubles_with_recipient = [](const auto &code) {
using namespace grpc;
std::vector<int> codes = {StatusCode::CANCELLED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::vector<int> codes = {StatusCode::CANCELLED,
std::set<StatusCode> codes = {StatusCode::CANCELLED,

performs better lookup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that set is more appropriate because it is a more reliable way to stay unique constants.

TEST_F(BasicMstPropagationFixture,
MstStateOfTransactionWithoutAllSignaturesPropagtesToOtherPeer) {
DISABLED_MstStateOfTransactionWithoutAllSignaturesPropagtesToOtherPeer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the test disabled? Please provide a reason. Taking into account another failing test related to MST (see CI), I suggest possible investigating MST regression - it could cause this test failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems, the branch was outdated and now basic_mst_state_propagation test absent.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants