10000 Adding circuit breaker internal module by ggaurav08 · Pull Request #4335 · m3db/m3 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adding circuit breaker internal module #4335

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Adding circuit breaker internal module #4335

wants to merge 11 commits into from

Conversation

ggaurav08
Copy link
@ggaurav08 ggaurav08 commented Apr 25, 2025 •
edited
Loading

What this PR does / why we need it:
This PR adds circuit breaker core library that provides APIs for a failure-rate based circuit breaking for high throughput systems. It is referenced from open source yarpc module, that is also used in uber go monorepo.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

prateek
prateek reviewed Apr 28, 2025
View reviewed changes
Copy link
Collaborator
@prateek prateek left a comment

Choose a reason for hiding this comment

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

few things to address

  • nothing uses fx in this repo, lets not start now please. don't call this this circuitbreakerfx/...
  • move this to either src/dbnode/client/circuitbreaker or src/dbnode/x/circuitbreaker

prateek
prateek reviewed Apr 30, 2025
View reviewed changes
Copy link
Collaborator
@prateek prateek left a comment

Choose a reason for hiding this comment

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

re: src/dbnode/client/circuitbreaker/internal/circuitbreaker/circuit.go

  • don't worry about internal/ v not (the repo never adopted that idiom)
  • and why do you have circuitbreaker being a sub-package of circuitbreaker?

What's wrong with the simple version src/dbnode/client/circuitbreaker?

@prateek
Copy link
Collaborator
prateek commented Apr 30, 2025

also - please fix ci tests/lint

@ggaurav08 ggaurav08 marked this pull request as ready for review May 19, 2025 11:24
prateek
prateek approved these changes May 23, 2025
View reviewed changes
Copy link
Collaborator
@prateek prateek left a comment

Choose a reason for hiding this comment

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

lgtm

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

@prateek prateek prateek approved these changes

@ramnik-uber ramnik-uber Awaiting requested review from ramnik-uber

@arnav-chakraborty arnav-chakraborty Awaiting requested review from arnav-chakraborty

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ggaurav08 @CLAassistant @prateek
0