Live reload per-host clients #219
Conversation
Generate changelog in
|
| Some(e) => { | ||
| for e2 in errors { | ||
| warn!("error reloading per-host clients", error: e2); | ||
| } |
There was a problem hiding this comment.
The error handling here is all deferred so we don't lose track of state when a subset of clients have issues updating. I don't expect that to be a common case but wanted to make sure we handled it.
There was a problem hiding this comment.
Where are we updating the state in this method? As in, what would be lost if we returned the error early here https://github.com/palantir/conjure-rust-runtime/pull/219/files#diff-6d2572eabc17f7b3329877aef40ce0daeebbc8cb310e2c5394e530c28b6d42f4R446?
There was a problem hiding this comment.
We'd forget about some of the clients so the next refresh we'd end up making new ones for those hosts instead of reloading the old ones.
| override_host_index: Option<usize>, | ||
| ) -> Result<Client, Error> | ||
| where | ||
| T: Borrow<StateBuilder> + 'static + Sync + Send, |
There was a problem hiding this comment.
I'm trying to understand how this Borrow type works (and why it's useful vs. just using &T).
I read about it here https://doc.rust-lang.org/std/borrow/trait.Borrow.html. Is it sort of like String, in that you can just pass T directly, but it can be implicitly cast to &T (or &str) because it implements Borrow?
There was a problem hiding this comment.
It has some extra requirements that are used in some other places, but the key thing in this context is that it's a trait implemented by both T and Arc<T> that lets you get a &T. This function is called in 2 places. The per-host stuff needs to store the state builder in an arc, while the normal stuff up in client_inner doesn't and this bound makes both work.
| } | ||
|
|
||
| Ok(PerHostClients { clients }) | ||
| client_handles = new_client_handles; |
There was a problem hiding this comment.
It's used up on line 429.
There was a problem hiding this comment.
I think where I'm confused is this (line 457) is outside of the loop. So how does our reassignment here affect line 429?
There was a problem hiding this comment.
Oh yeah, it's not updating it for the next loop iteration, it's updating it for the next call to this closure (i.e. the next time the service config changes)
| fn raw_client<T>( | ||
| state_builder: T, | ||
| service_config: Refreshable<ServiceConfig, Error>, | ||
| override_host_index: Option<usize>, |
There was a problem hiding this comment.
I see this is part of the CachedConfig type. It says it Overrides the hostIndex field included in metrics. What is the purpose of that?
There was a problem hiding this comment.
There are some metrics which are tracked for each URI separately with a hostIndex field. Normally that's determined by the index of the URI in the URIs list of the ServiceConfig, but the per-host logic ends up making a separate ServiceConfig for each URI. Without the override each of those would end up using hostIndex 0 so the metrics would all get merged.
| Some(e) => { | ||
| for e2 in errors { | ||
| warn!("error reloading per-host clients", error: e2); | ||
| } |
There was a problem hiding this comment.
Where are we updating the state in this method? As in, what would be lost if we returned the error early here https://github.com/palantir/conjure-rust-runtime/pull/219/files#diff-6d2572eabc17f7b3329877aef40ce0daeebbc8cb310e2c5394e530c28b6d42f4R446?
|
Released 5.1.0 |
Before this PR
Each client in a per-host-clients context was static - its internal configuration would not live reload based on config changes even when that host was still present. This makes it hard to use in some contexts where there are background tasks happening on a per-host basis.
After this PR
==COMMIT_MSG==
Each per-host client now live reloads as long as that host remains in the service configuration.
==COMMIT_MSG==
Closes #218