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

Beginning of peerset implementation. #62

Merged
merged 4 commits into from
Oct 11, 2019
Merged

Beginning of peerset implementation. #62

merged 4 commits into from
Oct 11, 2019

Conversation

hdevalence
Copy link
Contributor

@hdevalence hdevalence commented Oct 10, 2019

So far this just has initial work following design discussion with @dconnolly this afternoon.

TODO:

  • peerset can receive new peers from service discovery
  • peerset can load balance over existing peers
  • discovery implementation exists
  • peerset backpressure creates new peers
  • peersets can be instantiated in connect
  • seems to connect to live network?

@hdevalence
Copy link
Contributor Author

We noticed that there's an advantage to having a generic Discover implemenation, namely that a Discover impl that produces PeerClients can be wrapped in one that produces clients with load information (which has a different concrete type). However that means that the PeerSet cannot feed produce-a-new-peer requests directly via a method call (as it cannot call any methods beyond those provided by the trait).

@hdevalence
Copy link
Contributor Author

Perhaps as an initial step, it would be good to separate the backpressure-drives-peerset-expansion component from this PR and deal with that separately and subsequently, which would allow using, e.g., the ServiceStream Discover impl to start instead of writing our own.

@hdevalence
Copy link
Contributor Author

The "wip" commit stubs above change connect to build a peerset using a collection of peers from a single getaddr request. If these were cleaned up they would tick the last two boxes above, leaving only the custom Discover implementation and the auto-expansion of the peerset. Perhaps those two items would be better in a subsequent issue / PR, since doing them properly requires making calls to the peerset?

// XXX hack
impl Into<crate::BoxedStdError> for PeerError {
fn into(self) -> crate::BoxedStdError {
Box::new(format_err!("dropped error info").compat())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ultimately because failure::Error doesn't implement Clone; fixing it properly requires reworking other error handling (to use custom Fails) and I would like to wait two weeks to do it rather than do it now, if that's OK with others

@hdevalence hdevalence changed the title WIP Peerset Beginning of peerset implementation. Oct 10, 2019
@hdevalence hdevalence marked this pull request as ready for review October 10, 2019 22:38
@hdevalence
Copy link
Contributor Author

Per slack discussion, dropping the middle two tasks for a subsequent PR.

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

LGTM ✅

hdevalence and others added 4 commits October 10, 2019 17:58
@hdevalence
Copy link
Contributor Author

Rebased onto main

@hdevalence hdevalence merged commit ae1a164 into main Oct 11, 2019
@hdevalence hdevalence deleted the peerset branch October 11, 2019 01:15
skyl added a commit to skyl/zebra that referenced this pull request Sep 25, 2024
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