Add routing reload option #17
Conversation
| @@ -30,11 +35,11 @@ macro_rules! try_client { | |||
|
|
|||
| pub struct Hopper { | |||
| metrics: Arc<Metrics>, | |||
| router: Arc<dyn Router>, | |||
| router: Arc<Mutex<Arc<dyn Router>>>, | |||
There was a problem hiding this comment.
- Adding a Mutex here would highly impact performance. I specifically chose to not use a Mutex here in the design of hopper
- There's no need for a double Arc
| let server = Hopper::new(Arc::new(config.routing), metrics); | ||
| let server = Hopper::new(Arc::new(Mutex::new(Arc::new(config.routing))), metrics); | ||
|
|
||
| server.listen_config(signal(SignalKind::hangup()).expect("Unable to get signal")); |
There was a problem hiding this comment.
Instead of spawning a new task you could select the future on L46 alongside the stop signal await.
| loop { | ||
| stream.recv().await; | ||
|
|
||
| let config = ServerConfig::read().expect("Unable to read config"); |
There was a problem hiding this comment.
When expect panics the task stops but the process lives on, thus making hopper incapable of reloading new configurations.
|
I've found a crate to swap configurations without a Mutex: If you please could, rewrite the code using this crate, and also thank you for making the pr. |
|
Now that I think of it, you can just stop this future, make the changes to hopper and start it again. Dropping a TcpListener does not drop its streams, furthermore, listen doesn't take ownership but only a shared reference so yea dropping the future also drops the borrow and the original Hopper instance you called |
|
Hey, So, I currently have lots of stuff to do for my network and therefore haven't had and probably will not have the time to finish this PR in the coming days. |
|
Yea np I think I鈥檒l handle this one myself. Btw are you planning on using hopper for your network by any chance? |
Yea, my plan was to replace HaProxy with hopper. |
Your solution is fine and works and you could use your fork for the moment. I have completed implementing it how I intended and just need to restyle a few things, will probably commit and release by tomorrow. Glad to hear that btw |
Hey
I tried my best to implement my feature from #16.
However, since I haven't worked a lot in rust, yet it may be not up to the standard needed. So feel free to change it as needed or just reject it.
Wherever you reject or accept the PR, I would appreciate some feedback 馃槃