Skip to content

Implement circuit breaker#129

Open
MitulShah1 wants to merge 8 commits intolabstack:masterfrom
MitulShah1:implement-circuit-breaker
Open

Implement circuit breaker#129
MitulShah1 wants to merge 8 commits intolabstack:masterfrom
MitulShah1:implement-circuit-breaker

Conversation

@MitulShah1
Copy link

This PR contains Circuit Breaker Middleware

@aldas @jrhrmsll @lammel Would appreciate it if you can review this PR.

@MitulShah1
Copy link
Author

MitulShah1 commented Mar 28, 2025

@aldas I've made changes and make it more advance and optimize, can you please check again?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not seem to be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually Go does not use getter naming. So State() would be more idiomatic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these stats should have their own struct with field and their types. returning map of interfaces looks like something that belongs to javascript world.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be if cb.state == StateOpen { return} to get rid of 1 level of indentation

@MitulShah1
Copy link
Author

@aldas Thanks for detail review, I made below changes

  1. Removed Unused context
  2. Instead of a channel-based semaphore, Added atomic counter operations
  3. Timestamp-based state transition instead of using timers.
  4. Updated the locking strategy in AllowRequest()
  5. Simplified resource management
  6. Updated README File

Please review now.

@MitulShah1
Copy link
Author

@aldas Can you please review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments