Implement circuit breaker #129
Conversation
|
@aldas I've made changes and make it more advance and optimize, can you please check again? |
circuitbreaker/circuit_breaker.go
Outdated
| timeout time.Duration // Duration to stay open before transitioning to half-open | ||
| successThreshold int // Successes required to close circuit | ||
| openTimer *time.Timer // Timer for state transition from open to half-open | ||
| ctx context.Context // Context for cancellation |
There was a problem hiding this comment.
this does not seem to be used.
circuitbreaker/circuit_breaker.go
Outdated
| cancel context.CancelFunc // Cancel function for cleanup | ||
| config Config // Configuration settings | ||
| now func() time.Time // Function for getting current time (useful for testing) | ||
| halfOpenSemaphore chan struct{} // Controls limited requests in half-open state |
There was a problem hiding this comment.
why not use atomic.uint to count requests in flight? At instead of that openTimer *time.Timer just have atomic uint64 (timestamp nano) that is checked if halfstate has been expired.
or atleast move this semaphore specific logic to own struct + methods that are similar to https://pkg.go.dev/golang.org/x/sync/semaphore API - this would be easier to read.
or very least - all places where there is select { in this code. it must have comment what it does. You do not see often people doing semaphores with chans and it takes too much processing/mental power to understand what the codes does. Especially when it comes to maintaining. For example we Echo maintainers have to read this code in 6, 12, 24 months and understand what it does with minimal effort. I can not expect you to be around then to explain.
circuitbreaker/circuit_breaker.go
Outdated
| } | ||
|
|
||
| // GetState returns the current state of the circuit breaker | ||
| func (cb *CircuitBreaker) GetState() State { |
There was a problem hiding this comment.
Usually Go does not use getter naming. So State() would be more idiomatic.
circuitbreaker/circuit_breaker.go
Outdated
| func (cb *CircuitBreaker) AllowRequest() (bool, State) { | ||
| atomic.AddInt64(&cb.totalRequests, 1) | ||
|
|
||
| cb.mutex.RLock() |
There was a problem hiding this comment.
I think guarding this small block with read lock makes little difference. This whole method should be behind read lock if it is necessary that limits are honored/guarded against races during extreme peaks.
circuitbreaker/circuit_breaker.go
Outdated
| func (cb *CircuitBreaker) GetStateStats() map[string]interface{} { | ||
| state := cb.GetState() | ||
|
|
||
| return map[string]interface{}{ |
There was a problem hiding this comment.
these stats should have their own struct with field and their types. returning map of interfaces looks like something that belongs to javascript world.
circuitbreaker/circuit_breaker.go
Outdated
| } | ||
|
|
||
| // HealthHandler returns an Echo handler for checking circuit breaker status | ||
| func (cb *CircuitBreaker) HealthHandler() echo.HandlerFunc { |
There was a problem hiding this comment.
I think this is little bit overthinking - if there is a method that returns stats when everyone can easily build their own handlers. assume that there will be people that want to have their own structure for output.
circuitbreaker/circuit_breaker.go
Outdated
| cb.mutex.Lock() | ||
| defer cb.mutex.Unlock() | ||
|
|
||
| if cb.state != StateOpen { |
There was a problem hiding this comment.
this could be if cb.state == StateOpen { return} to get rid of 1 level of indentation
This PR contains Circuit Breaker Middleware
@aldas @jrhrmsll @lammel Would appreciate it if you can review this PR.