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

[WIP] Add a Namer interface #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP] Add a Namer interface #80

wants to merge 2 commits into from

Conversation

adleong
Copy link
Member

@adleong adleong commented Sep 13, 2017

This change hides Namerd behind a Namer interface so that the Namer can be dynamically configurable. I'm looking for feedback on the general approach taken here, as well as code style and idioms.

The main challenge I ran into was making the builder pattern polymorphic. An executor contains something on which I can call with_handle which returns something on which I can call resolve which returns something that implements Stream. Furthermore, I think dynamic dispatch is necessary since the actual choice of implementation will be dynamically determined by a config file.

I do not plan to merge this as is. After getting feedback on this I will remove the namerd implementation and replace it with a static namer.

@adleong adleong self-assigned this Sep 13, 2017
@olix0r
Copy link
Member

olix0r commented Sep 13, 2017

i should note, i'm more than happy to make larger changes to linkerd-tcp to accommodate a better solution

@@ -31,12 +33,12 @@ pub type Result<T> = ::std::result::Result<T, Error>;
///
/// The `Resolver` side is a client of the `Executor`. Namerd work is performed on
/// whatever thread the executor is spawned on.
pub fn new(namerd: Namerd) -> (Resolver, Executor) {
pub fn new<N: Namer + 'static + Send>(namer: N) -> (Resolver, Executor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, using where clauses for long bounds like this can be more readable.

pub fn new<N>(namer: N) -> (Resolver, Executor) 
where N: Namer + Send,
      N: 'static,

Copy link
Member Author

Choose a reason for hiding this comment

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

what's the guidance on how to group these or break them up? I notice you have Namer + Send together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I decided to group the trait bounds separately from the lifetime bound, but

where N: Namer,
      N: Send,
      N: 'static,

would also be acceptable – I don't think there's a strongly codified guideline here.

}

impl Executor {
impl Executor where {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this compile? this doesn't look like something that would compile.

Copy link
Member Author

Choose a reason for hiding this comment

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

believe it or not, it does

Copy link
Contributor

@hawkw hawkw Sep 13, 2017

Choose a reason for hiding this comment

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

what does the empty where clause do, then? I've never seen this before.

Copy link
Member Author

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ this where was accidentally left in. but somehow it does compile!

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, okay, I guess the empty where does nothing then. Makes sense.

@@ -77,16 +79,16 @@ impl Stream for Resolve {
/// Serves resolutions from `Resolver`s.
pub struct Executor {
requests: mpsc::UnboundedReceiver<(Path, mpsc::UnboundedSender<Result<Vec<WeightedAddr>>>)>,
namerd: Namerd,
namer: Box<Namer + Send>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you always want the Send bound on Namer, you might want to consider making it part of the trait, instead:

pub trait Namer: Send {
    ...
}

@@ -31,12 +33,12 @@ pub type Result<T> = ::std::result::Result<T, Error>;
///
/// The `Resolver` side is a client of the `Executor`. Namerd work is performed on
/// whatever thread the executor is spawned on.
pub fn new(namerd: Namerd) -> (Resolver, Executor) {
pub fn new<N: Namer + 'static + Send>(namer: N) -> (Resolver, Executor) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm very curious about the 'static lifetime constraint here. The intention of this function is that the Namer is moved into this function, boxed, and the box is moved into the Executor. I am surprised that lifetimes come into play because, as far as I understand, Namer the just being moved around, not borrowed.

'static indicates that the Namer will live forever? But since it is owned by the Executor, isn't it's lifetime the same as the Executor?

B(B),
}

impl<A, B> Stream for StreamEither<A, B>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cute - I bet we could easily generalize this to make an Either that implements any trait implemented by both A and B?

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

🚫📦! this is cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants