Conversation
linkerd/stack/src/watch.rs
Outdated
| use tracing::Instrument; | ||
|
|
||
| pub trait UpdateWatch<T> { | ||
| type Service: Clone + Default; |
There was a problem hiding this comment.
i put the Clone and Default bounds here because we'll get a much more obvious type error (when trying to implement UpdateWatch, rather than on the NewService impl) if they're missing
There was a problem hiding this comment.
we probably should also put the Send + Sync bounds here since this trait is only used by NewSpawnWatch anyway...
Since `UpdateWatch` is only ever used by `SpawnWatch`, and `SpawnWatch` requires these bounds, they may as well go on the trait directly. This way, we'll get much more reasonable errors when trying to implement `UpdateWatch`, rather than just having `NewService` impls fail to exist.
hawkw
left a comment
There was a problem hiding this comment.
it's neat that the watch logic can be factored out, this looks good to me! i left a few minor nits/questions, but this looks good overall!
| .push(svc::NewLazy::layer()), | ||
| let route = svc::layers() | ||
| // Maintain a per-route distributor over concrete | ||
| // backends from the (above) concrete cache. |
There was a problem hiding this comment.
nit: is the cache still above this (in the file)? does the comment need to be updated?
linkerd/distribute/src/service.rs
Outdated
| } | ||
|
|
||
| impl<K, S> Default for Distribute<K, S> { | ||
| /// Returns an empty distribution. This distribution will never become ready/ |
There was a problem hiding this comment.
whoops, typo on my part:
| /// Returns an empty distribution. This distribution will never become ready/ | |
| /// Returns an empty distribution. This distribution will never become ready. |
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct NewSpawnWatch<P, N> { |
There was a problem hiding this comment.
probably worth a comment somewhere in the file explaining what this does?
| fn update(&mut self, target: &T) -> Option<Self::Service>; | ||
| } | ||
|
|
||
| #[derive(Clone, Debug)] |
There was a problem hiding this comment.
this will only implement Clone/Debug if P is Clone/Debug, because of the PhantomData...should we have a manual impl?
| type Future = S::Future; | ||
|
|
||
| fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
| if matches!(self.rx.has_changed(), Ok(true)) { |
There was a problem hiding this comment.
should poll_ready error if the background task is dead? AFAICT it could disappear on us abruptly if the underlying target watch closes, and wouldn't be recoverable (we would need to start a new resolution)...
There was a problem hiding this comment.
I don't think there's any need to error--we have a handle to the service we can continue using.
This branch adds an implementation of a middleware that watches a `watch::Receiver` of targets and builds a new service when the watch is updated. The current state of the inner service is broadcast over a second watch to any clones of the `SpawnWatch` service. This is the same code added in PR #2100, but rebased onto `main`, with documentation and tests added.
No description provided.