[WIP] Add a Namer interface #80
Conversation
|
i should note, i'm more than happy to make larger changes to linkerd-tcp to accommodate a better solution |
src/resolver/mod.rs
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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,There was a problem hiding this comment.
what's the guidance on how to group these or break them up? I notice you have Namer + Send together.
There was a problem hiding this comment.
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.
src/resolver/mod.rs
Outdated
| } | ||
|
|
||
| impl Executor { | ||
| impl Executor where { |
There was a problem hiding this comment.
does this compile? this doesn't look like something that would compile.
There was a problem hiding this comment.
believe it or not, it does
There was a problem hiding this comment.
what does the empty where clause do, then? I've never seen this before.
There was a problem hiding this comment.
¯_(ツ)_/¯ this where was accidentally left in. but somehow it does compile!
There was a problem hiding this comment.
ah, okay, I guess the empty where does nothing then. Makes sense.
src/resolver/mod.rs
Outdated
| @@ -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>, | |||
There was a problem hiding this comment.
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 {
...
}
src/resolver/mod.rs
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
This is cute - I bet we could easily generalize this to make an Either that implements any trait implemented by both A and B?
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_handlewhich returns something on which I can callresolvewhich 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.