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

Remove ConnectionRequest/Response - Part 1 #1868

Merged
merged 11 commits into from Oct 29, 2019

Conversation

@octol
Copy link
Contributor

octol commented Oct 28, 2019

Extracted out the first part of #1865 together with the first steps of removing ConnectionRequest/Response

Part of #1855

@octol octol requested review from jeanphilippeD and madadam Oct 28, 2019
@octol octol marked this pull request as ready for review Oct 28, 2019
@octol octol referenced this pull request Oct 28, 2019
3 of 5 tasks complete
Copy link
Contributor

jeanphilippeD left a comment

Ok, beside the problem with clippy, that Part 1 seem to mostly address the problem we were trying to solve removing all the call to send_connection_request except the corner case when loosing a node.

Copy link
Contributor

jeanphilippeD left a comment

Additional change look good.

@octol octol force-pushed the octol:try_rm_req branch from 21c5d43 to e94e057 Oct 29, 2019
Copy link
Contributor

madadam left a comment

Some mostly minor comments. Also, do we still need Approved::handle_connection_request and Approved::send_connection_request? And MessageContent::ConnectionRequest and DirectMessage::ConnectionResponse?

};
let p2p_nodes = ids
.iter()
.map(|full_id| P2pNode::new(*full_id.public_id(), connection_info.clone()))

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

It might not matter for this test, but it's a bit weird to use the same ConnectionInfo for all the nodes.

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Suggestion:

let p2p_nodes = ids.iter()
    .enumerate()
    .map(|(index, full_id)| {
        P2pNode::new(*full_id.public_id(), ConnectionInfo { peer_addr: ([127, 0, 0, 1], index + 9000).into(),  peer_cert_der: vec![] })
    })
    .colllect()

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

Thanks, that's good suggestion, I'll update!

.state
.our_members
.entry(pub_id)
.or_insert_with(|| MemberInfo::new(p2p_node.connection_info().clone()));

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Consider adding P2pNode::into_connection_info to avoid this clone.

// We already have the connection info from when it was added online.
let connection_info = self
.get_member_connection_info(&pub_id)
.ok_or(RoutingError::InvalidStateForOperation)?;

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

InvalidStateForOperation doesn't seem to right error to return here, although we don't currently have anything better. I'd consider repurposing the InvalidConnection error for this, but renaming it to InvalidPeer or PeerNotFound or something like that.

This applies to remove_elder too.

// WIP: remove me by the end of this PR
let mut elders = self.state.new_info.members();
let _ = elders.insert(pub_id);
Comment on lines +427 to 429

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Why do we still need this?

let connection_info = self
.get_member_connection_info(&pub_id)
.ok_or(RoutingError::InvalidStateForOperation)?;
let p2p_node = P2pNode::new(pub_id, connection_info);

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Seems you only construct this P2pNode to be able to remove it from the elders collection. There are at least two ways to avoid doing this:

  1. Change p2p_members from BTreeSet<P2pNode> to BTreeMap<PublicId, ConnectionInfo>
  2. Implement Borrow<PublicId> for P2pNode and then just do elders.remove(&pub_id) (see https://doc.rust-lang.org/std/collections/struct.BTreeSet.html#method.remove). Note for this to work properly, the Ord impl of P2pNode must be the same as that of PublicId, but it seems that is already the case.

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

Oh nice, I'll go with the section option of implementing Borrow<PublicId>

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Nov 1, 2019

Contributor

As a note, the Borrow seem to be a bit advanced since it put some implicit constraint on the Ord. I'd probably go with either BTreeMap<PublicId, ConnectionInfo>, or BTreeMap<PublicId, P2pNode>, as that should be sufficient and more explicit that the ordering is only on the PublicId.

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

I take it you mean something like adding

    pub fn member_connection_infos(&self) -> BTreeMap<&PublicId, &ConnectionInfo> {
        self.members
            .iter()
            .map(|p2p_node| (p2p_node.public_id(), p2p_node.connection_info()))
            .collect()
    }

to EldersInfo and then in Chain do

        let mut elders = self.state.new_info.member_connection_infos();
        let _ = elders.remove(&pub_id);
        let p2p_elders = elders
            .into_iter()
            .map(|(id, conn_info)| P2pNode::new(*id, conn_info.clone()))
            .collect();

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Nov 1, 2019

Contributor

No, I do not mean add a new map, I mean replace what is a set with a map. (not a map of reference).
But maybe I'm missing something? (could HO to look at code together).

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

You could change the above member_connection_infos to return BTreeMap<PublicId, ConnectionInfo>, but I'm not sure you'd gain much outside giving the caller no choice about the clone?

For reference the original code called p2p_members(&self) -> &BTreeSet<P2pNode>

let conn_infos: Vec<_> = p2p_nodes
.iter()
.map(|n| n.connection_info().clone())
.collect();
info!(
"{} - Joining a section {:?}: {:?}",
self, prefix, conn_infos
);
Comment on lines +347 to 354

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Just pass the full p2p_nodes into the info, not need to collect into conn_infos first. It's going to be more useful and more efficient.

if !self.peer_map.has(&pub_id) {
self.peer_map
.insert(pub_id, p2p_node.connection_info().clone());
self.send_direct_message(&pub_id, DirectMessage::ConnectionResponse);

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Do we still need to do this? The neighbours are going to receive a NeighbourInfo from us which would also contain our P2pNode, so they will insert us into their peer_map when they process it.

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

In the follow up PR we're going to try do remove the ConnectionResponse messages entirely, so leaving this as is for now

let to_connect: Vec<_> = self
.chain
.our_elders()
.filter(|p2p_node| !self.peer_map.has(p2p_node.public_id()))
.cloned()
.collect();

for p2p_node in to_connect.into_iter() {
let pub_id = p2p_node.public_id();
self.peer_map
.insert(*pub_id, p2p_node.connection_info().clone());
self.send_direct_message(pub_id, DirectMessage::ConnectionResponse);
}
}
Comment on lines +359 to 372

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

I think this whole thing can be replaced with just

for p2p_node in self.chain.our_elders() {
    self.peer_map.insert(*p2p_node.public_id(), p2p_node.connection_info().clone());
}

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

See previous comment, I hope to remove this entirely in the upcoming PR

let _ = self.send_connection_request(pub_id, src, dst, outbox);
self.peer_map
.insert(pub_id, p2p_node.connection_info().clone());
self.send_direct_message(&pub_id, DirectMessage::ConnectionResponse);

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

Same here - why do we still need this?

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

Same as previous comment. I agree we want to remove these

.p2p_nodes
.iter()
.map(|p2p_node| p2p_node.connection_info().clone())
.collect();
for dst in conn_infos {
info!("{} - Sending JoinRequest to {:?}", self, dst);

This comment has been minimized.

Copy link
@madadam

madadam Oct 29, 2019

Contributor

It would be more useful to log the whole P2pNode here.

This comment has been minimized.

Copy link
@octol

octol Nov 1, 2019

Author Contributor

Yep agree. I think this whole conn_info lookup will go away though in the upcoming PR

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor

jeanphilippeD commented Oct 29, 2019

Created issue #1869 to track the additional review comments because other PR already depend on it.

Copy link
Contributor

madadam left a comment

Approving, conditioned by this issue getting addressed in a later PR: #1869

@jeanphilippeD jeanphilippeD merged commit 0fa3f01 into maidsafe:fleming Oct 29, 2019
16 checks passed
16 checks passed
Check (ubuntu-latest)
Details
Test (ubuntu-latest)
Details
Check (windows-latest)
Details
Test (windows-latest)
Details
Check (macOS-latest)
Details
Test (macOS-latest)
Details
Test (ubuntu-latest, --release --features mock)
Details
Test (ubuntu-latest, --release)
Details
Test (windows-latest, --release --features mock)
Details
Test (windows-latest, --release)
Details
Test (macOS-latest, --release --features mock)
Details
Test (macOS-latest, --release)
Details
Rustfmt
Details
Clippy
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jeanphilippeD added a commit that referenced this pull request Nov 5, 2019
Review comment fixes for #1868
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.