Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace AcceptAsCandidate RPC #1622

Merged

Conversation

Projects
None yet
3 participants
@jeanphilippeD
Copy link

commented May 1, 2019

Add:
-ExpectCandidate struct from MessageContent::ExpectCandidate.
-NetworkEvent::ExpectCandidate to consensus the RPC and the
RPC destination used as the source of the response.

Remove:
-MessageContent::AcceptAsCandidate replacing its function with consensus
through Parsec of NetworkEvent::ExpectCandidate.
-Candidate::Expecting that was used when waiting AcceptAsCandidate.
-CANDIDATE_ACCEPT_TIMEOUT_SECS

Replace:
-expect_candidate with has_candidate: Do not raise an error in handling
the parsec event because we already have a candidate.

Update handling ExpectCandidate:
-handle_expect_candidate only vote for the received RPC.
-new handle_expect_candidate_event take on combined responsabilities
of handle_expect_candidate and handle_expect_candidate_event.
-only processed events if member of chain or established to be allowed
to vote.

@Viv-Rajkumar
Copy link
Member

left a comment

hmm looking good 👍

I guess it should be fine to carry over NetworkEvent::ExpectCandidate into the new prefix, just confirming if it was intentional in this PR.

pub dst_name: XorName,
}

impl ExpectCandidateVote {

This comment has been minimized.

Copy link
@Viv-Rajkumar

Viv-Rajkumar May 1, 2019

Member

still not a fan of this being a type by itself than just defined within NetworkEvent directly

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD May 1, 2019

Author

It makes passing it around a lot easier though, and I do pass it around.
But I could probably mostly pass arround ExpectCandidate if that is where we want to go.

This comment has been minimized.

Copy link
@Viv-Rajkumar

Viv-Rajkumar May 1, 2019

Member

yeh its fair play if we pass it around / you guys prefer it, but the name still is kind of weird considering its NetworkEvent we vote for in parsec from routing and this is a type within that enum and also called ..Vote.

In this case the impl is also sorta weird to me cos we only use into_network_event I think once and if we constructed it directly, could have saved this entire impl definition. I'm prolly just biased to more primitive types that way instead of wrapper code.

Show resolved Hide resolved src/chain/network_event.rs Outdated
Show resolved Hide resolved src/states/node.rs Outdated
Show resolved Hide resolved src/states/node.rs
&mut self,
vote: ExpectCandidateVote,
) -> Result<(), RoutingError> {
if !self.chain.is_member() || !self.peer_mgr.is_established() {

This comment has been minimized.

Copy link
@Viv-Rajkumar

Viv-Rajkumar May 1, 2019

Member

same as prev comment

@jeanphilippeD

This comment has been minimized.

Copy link
Author

commented May 1, 2019

just confirming if it was intentional in this PR.

No, it was not intentional. However, as you say, it is ok, though not unlikely to not accumulate if too many nodes in the new prefix did not receive the RPC.
I think it make sense to not add special handling in this PR.

@Viv-Rajkumar

This comment has been minimized.

Copy link
Member

commented May 1, 2019

just confirming if it was intentional in this PR.

No, it was not intentional. However, as you say, it is ok, though not unlikely to not accumulate if too many nodes in the new prefix did not receive the RPC.
I think it make sense to not add special handling in this PR.

hmm I wouldn't see this as special case handling normally. If anything what we have with https://github.com/maidsafe/routing/blob/fleming/src/states/node.rs#L934 is not great thus allowing this to have not get picked up compile time. If we matched there explicitly, introducing a new NetworkEvent would have forced us to make the choice explicitly here.

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:remove_accept_as_candidate_rpc branch 4 times, most recently from 3b4889a to 434a0d4 May 2, 2019

@pierrechevalier83
Copy link
Contributor

left a comment

I've left a few comments, but they're mostly nitpicks as I stared at the code very hard while getting the bigger picture.
Overall, seems inline with the RFC and a good change overall as we're getting closer to a more consistent view between different nodes. Good work 😄

Show resolved Hide resolved src/states/node.rs Outdated
Show resolved Hide resolved src/states/node.rs Outdated
Show resolved Hide resolved src/states/node.rs Outdated
Show resolved Hide resolved src/states/node.rs Outdated
Show resolved Hide resolved src/messages/mod.rs Outdated
Show resolved Hide resolved src/peer_manager.rs
Show resolved Hide resolved src/states/node.rs
Show resolved Hide resolved src/states/node.rs Outdated
Show resolved Hide resolved src/states/node.rs
Show resolved Hide resolved src/states/node.rs Outdated

jeanphilippeD added some commits May 2, 2019

fix/routing: handle ExpectCandidate consistently with Parsec
Add:
-ExpectCandidate struct from MessageContent::ExpectCandidate.
-NetworkEvent::ExpectCandidate to consensus the RPC and the
 RPC destination used as the source of the response.

Remove:
-MessageContent::AcceptAsCandidate replacing its function with consensus
 through Parsec of NetworkEvent::ExpectCandidate.
-Candidate::Expecting that was used when waiting AcceptAsCandidate.
-CANDIDATE_ACCEPT_TIMEOUT_SECS

Replace:
-expect_candidate with has_candidate: Do not raise an error in handling
 the parsec event because we already have a candidate.

Update handling ExpectCandidate:
-handle_expect_candidate only vote for the received RPC.
-new handle_expect_candidate_event take on combined responsabilities
 of handle_expect_candidate and handle_expect_candidate_event.
-only processed events if member of chain or established to be allowed
 to vote.

Test:
5 iteration of the full tests including
mock_crust::churn::aggressive_churn with features=mock.
refactor/routing: refactor handling of ExpectCandidate
Keep high level functionality in top function and add sub functions
for the details.

Behaviour change:
- Only take away self.next_relocation_interval if we use it, not if we
  send candidate to other section.

Test:
5 iteration of the full tests including
mock_crust::churn::aggressive_churn with features=mock.

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:remove_accept_as_candidate_rpc branch from 434a0d4 to 69e3caa May 2, 2019

@pierrechevalier83
Copy link
Contributor

left a comment

As discussed, happy as is.

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:remove_accept_as_candidate_rpc branch from cb9d4a4 to fb7a376 May 2, 2019

test/routing: always handle at least one Relocating node
Verify that at least one node is added.

What we would want to check is at least one node is added for each
section that received at leat one ExpectCandidate RPC.

However this approach is very simple and also validate that statement
if there is only one section in the network: It only partial coverage
but failed without ExpectCandidate going through Parsec.

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:remove_accept_as_candidate_rpc branch from fb7a376 to d5a6582 May 2, 2019

@Viv-Rajkumar Viv-Rajkumar merged commit cf83857 into maidsafe:fleming May 3, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jeanphilippeD jeanphilippeD deleted the jeanphilippeD:remove_accept_as_candidate_rpc branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.