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

Add Circuit Breaker implementation #1738

Merged
merged 8 commits into from
Jul 25, 2022
Merged

Conversation

hlts2
Copy link
Contributor

@hlts2 hlts2 commented Jul 19, 2022

Description:

WHY

When checking backoff metrics with Grafana, I noticed a large number of unnecessary backoff retries.

WHAT

I developed the Circuit Breaker function to prevent a large number of unnecessary retries.

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.18.3
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 1.14.7

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
@hlts2 hlts2 self-assigned this Jul 19, 2022
@vdaas-ci
Copy link
Collaborator

[WARNING:INTCFG] Changes in interal/config may require you to change Helm charts. Please check.

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase master
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #1738 (8ce5e1a) into master (ed23300) will decrease coverage by 0.36%.
The diff coverage is 1.23%.

@@            Coverage Diff             @@
##           master    #1738      +/-   ##
==========================================
- Coverage   31.64%   31.27%   -0.37%     
==========================================
  Files         372      381       +9     
  Lines       32313    32698     +385     
==========================================
+ Hits        10224    10225       +1     
- Misses      21696    22071     +375     
- Partials      393      402       +9     
Impacted Files Coverage Δ
internal/circuitbreaker/breaker.go 0.00% <0.00%> (ø)
internal/circuitbreaker/counter.go 0.00% <0.00%> (ø)
internal/circuitbreaker/manager.go 0.00% <0.00%> (ø)
internal/circuitbreaker/state.go 0.00% <0.00%> (ø)
internal/circuitbreaker/tripper.go 0.00% <0.00%> (ø)
internal/config/circuitbreaker.go 0.00% <0.00%> (ø)
internal/config/grpc.go 87.89% <0.00%> (-12.11%) ⬇️
internal/errors/circuitbreaker.go 0.00% <0.00%> (ø)
internal/net/grpc/client.go 0.00% <0.00%> (ø)
internal/net/grpc/option.go 4.95% <0.00%> (-0.12%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d6c622...8ce5e1a. Read the comment docs.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

golangci

internal/circuitbreaker/breaker.go|105 col 6| variable name 'ok' is too short for the scope of its usage (varnamelen)
internal/circuitbreaker/manager.go|48 col 6| variable name 'st' is too short for the scope of its usage (varnamelen)
internal/circuitbreaker/breaker.go|106 col 6| variable name 'st' is too short for the scope of its usage (varnamelen)
internal/circuitbreaker/options.go|95 col 14| parameter name 'b' is too short for the scope of its usage (varnamelen)
internal/circuitbreaker/options.go|112 col 14| parameter name 'b' is too short for the scope of its usage (varnamelen)
internal/observability/metrics/circuitbreaker/circuitbreaker.go|57 col 2| variable name 'ms' is too short for the scope of its usage (varnamelen)

internal/errors/circuitbreaker.go Show resolved Hide resolved
internal/errors/circuitbreaker.go Show resolved Hide resolved
internal/errors/circuitbreaker.go Show resolved Hide resolved
internal/circuitbreaker/breaker.go Show resolved Hide resolved
)

func (s State) String() string {
switch s {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
missing cases in switch of type State: StateUnknown (exhaustive)

internal/circuitbreaker/options.go Outdated Show resolved Hide resolved
internal/circuitbreaker/breaker.go Show resolved Hide resolved
internal/circuitbreaker/counter.go Show resolved Hide resolved
internal/circuitbreaker/manager.go Show resolved Hide resolved
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 21, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8ce5e1a
Status: ✅  Deploy successful!
Preview URL: https://0dda5d95.vald.pages.dev
Branch Preview URL: https://feature-add-circuit-breaker-.vald.pages.dev

View logs

Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
@github-actions github-actions bot removed the size/XL label Jul 21, 2022
return nil
}
tests := []test{
// TODO test cases
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
internal/errors/circuitbreaker_test.go:47: Line contains TODO/BUG/FIXME: "TODO test cases" (godox)

},
*/

// TODO test cases
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
internal/errors/circuitbreaker_test.go:59: Line contains TODO/BUG/FIXME: "TODO test cases" (godox)

return nil
}
tests := []test{
// TODO test cases
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
internal/errors/circuitbreaker_test.go:120: Line contains TODO/BUG/FIXME: "TODO test cases" (godox)

},
*/

// TODO test cases
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
internal/errors/circuitbreaker_test.go:132: Line contains TODO/BUG/FIXME: "TODO test cases" (godox)

return nil
}
tests := []test{
// TODO test cases
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
internal/errors/circuitbreaker_test.go:196: Line contains TODO/BUG/FIXME: "TODO test cases" (godox)


// Do invokes the breaker matching the given key.
func (bm *breakerManager) Do(ctx context.Context, key string, fn func(ctx context.Context) (interface{}, error)) (val interface{}, err error) {
var st State
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
variable name 'st' is too short for the scope of its usage (varnamelen)

cnt.onFail()

var ok bool
var st State
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
variable name 'st' is too short for the scope of its usage (varnamelen)

cnt := b.count.Load().(*count)
cnt.onFail()

var ok bool
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
variable name 'ok' is too short for the scope of its usage (varnamelen)

// WithOpenTimeout returns an option that sets the timeout of "Open" state.
// After this period, the state will be changed from "Open" to "HalfOpen".
func WithOpenTimeout(timeout string) BreakerOption {
return func(b *breaker) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
parameter name 'b' is too short for the scope of its usage (varnamelen)

// WithClosedRefreshTimeout returns an option that sets the timeout of "Closed" state.
// After this period, the counter will be refreshed.
func WithClosedRefreshTimeout(timeout string) BreakerOption {
return func(b *breaker) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
parameter name 'b' is too short for the scope of its usage (varnamelen)

Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit 32b0f83 into master Jul 25, 2022
@kpango kpango deleted the feature/add-circuit-breaker-function branch July 25, 2022 07:32
type args struct {
opts []BreakerOption
}
type want struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
type want is unused (unused)

@kevindiu kevindiu mentioned this pull request Oct 4, 2022
@hlts2 hlts2 changed the title Add circuit breaker implementation Add Circuit Breaker implementation Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants