Conversation
| } | ||
| }; | ||
|
|
||
| let circuit_breaker = CircuitBreaker::new(circuit_breaker_config); |
There was a problem hiding this comment.
issue:
this change makes circuit-breaker being dropped each time it detects backend unhealthy, which breaks its logic around threshold_healtlhy.
There was a problem hiding this comment.
Which part of the logic seems broken to you? The circuit breaker restarts along with the proxies so it seemed fine to me
| } | ||
|
|
||
| _ = canceller.cancelled() => { | ||
| _ = reset_signal.cancelled() => { |
There was a problem hiding this comment.
issue:
the whole point of circuit-breaker is that it it's initialised once and runs for the whole duration of the process observing the status of the backend, applying threshold for healthy/unhealthy, and resetting the proxies when necessary.
this line effectively makes it self-terminate each time it detects backend unhealthy.
There was a problem hiding this comment.
But it restarts when it spins up new backends. This seems functionally equivalent to me and it makes sense that the lifetime of the circuit breaker is tied to the lifetime of the services it's managing.
There was a problem hiding this comment.
circuit-breaker can not properly apply "healthy" threshold if it's being restarted together with the proxies
There was a problem hiding this comment.
Can you elaborate? I could make the circuit break hold onto an Arc<Mutex<Option>> instead but I'd rather avoid adding mutability. Why can it not apply the healthy threshold if it's restarted with the proxies?
There was a problem hiding this comment.
after circuit-breaker marks backend as unhealthy it is supposed to count how many probes succeed consecutively and only once the threshold is reached it should flag backend as healthy again.
if it is restarted together with proxies, then it immediately considers the backend healthy (as if healthy threshold is always 0).
There was a problem hiding this comment.
Is this the only issue? It sounds like I should just initialize the circuit breaker with curr_status = Status::Unhealthy upon restarts
There was a problem hiding this comment.
i.d.k. that's the one that clearly pops up.
re-initialising the circuit-breaker from its past incarnation sounds way too artificial. what is the added value vs. the pattern that already exists: namely persistent circuit-breaker with a channel to send reset signals over?
the point of ditching the channel in favour of cancellation signal was to simplify things. now they don't look any simpler, and not even complex - but complicated instead.
2 participants
Extremely minimal change. Only changes the channel usage so a cancel token is used instead