Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace circuit breaker with library #98

Merged

Conversation

KianYang-Lee
Copy link
Contributor

@KianYang-Lee KianYang-Lee commented Jun 24, 2024

Previous implementation of circuit breaker was simplistic. It is replaced with a third-party library "gobreaker/v2". The function signature "recoverAfter" was removed to fit the implementation of the library. Test cases for circuit breaker were modified accordingly.

Closes #77

Previous implementation of circuit breaker was simplistic. It is replaced
with a third-party library "gobreaker/v2". The function signature
"recoverAfter" was removed to fit the implementation of the library.
Test cases for circuit breaker were modified accordingly.

lock := &sync.RWMutex{}
semiOpenLock := &sync.Mutex{}
func NewCircuitBreakerMiddleware(threshold uint, period time.Duration) func(next wasabi.RequestHandler) wasabi.RequestHandler {
Copy link
Owner

@ksysoev ksysoev Jun 24, 2024

Choose a reason for hiding this comment

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

Let's update type of threshold to uint32 to be in consistent.

Comment on lines 52 to 53
if err != nil {
return err
Copy link
Owner

Choose a reason for hiding this comment

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

Could you check for gobreaker.ErrOpenState error and return instead ErrCircuitBreakerOpen to hide dependency on the gobreaker... to have possibility to replace library in the future.

"time"

"github.com/ksysoev/wasabi"
"github.com/ksysoev/wasabi/dispatch"
"github.com/sony/gobreaker/v2"
)

type CircuitBreakerState uint8
Copy link
Owner

Choose a reason for hiding this comment

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

Probably this type is not needed anymore.

Copy link
Owner

@ksysoev ksysoev left a comment

Choose a reason for hiding this comment

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

@KianYang-Lee left few comments, rest looks good

Copy link
Owner

@ksysoev ksysoev left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for your contribution

@ksysoev ksysoev merged commit 88991aa into ksysoev:main Jun 25, 2024
2 checks passed
@KianYang-Lee KianYang-Lee deleted the feature/replace-circuit-breaker-with-library branch June 25, 2024 02:05
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.

Replace naive implementation of circuit breaker with using proper library
2 participants