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

Add NetworkBehaviour::inject_replaced #914

Merged
merged 5 commits into from
Feb 4, 2019
Merged

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Feb 4, 2019

Instead of calling "disconnected" followed with "connected", allows implementers to override an inject_replaced method.

The Kademlia behaviour needs this so that it can re-send the RPC query without marking the node as unreachable.

@ghost ghost assigned tomaka Feb 4, 2019
@ghost ghost added the in progress label Feb 4, 2019
Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Just a Q about the increased code complexity.

@@ -191,6 +191,28 @@ fn build_struct(ast: &DeriveInput, data_struct: &DataStruct) -> TokenStream {
})
};

// Build the list of statements to put in the body of `inject_replaced()`.
let inject_replaced_stmts = {
let num_fields = data_struct.fields.iter().filter(|f| !is_ignored(f)).count();
Copy link
Contributor

Choose a reason for hiding this comment

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

are you doing this count and all the rather complex looking code below only to save three clones? Are these three structs so heavy that this warrants the extra looping and increased code complexity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! The plan is to ultimately remove this custom derive, which is why no effort has been put into making its code more readable.

}
} else {
match field.ident {
Some(ref i) => quote!{ self.#i.inject_replaced(peer_id.clone(), closed_endpoint.clone(), new_endpoint.clone()); },
Copy link
Contributor

Choose a reason for hiding this comment

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

please wrap - this and the line below.

@tomaka
Copy link
Member Author

tomaka commented Feb 4, 2019

I pushed another commit that adds a call to set_disconnected which I forgot to put 🤦‍♂️

@tomaka
Copy link
Member Author

tomaka commented Feb 4, 2019

Also pushed another commit that adds the incoming address to kbuckets.
Kademlia really needs more testing after #913

@tomaka tomaka merged commit c9b7e23 into libp2p:master Feb 4, 2019
@ghost ghost removed the in progress label Feb 4, 2019
@tomaka tomaka deleted the inject-replaced branch February 4, 2019 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants