- Notifications
You must be signed in to change notification settings - Fork126
Circuit breaker changes using pybreaker#705
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Thanks for your contribution! To satisfy the DCO policy in ourcontributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Thanks for your contribution! To satisfy the DCO policy in ourcontributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
| ) | ||
| classCircuitBreakerStateListener(CircuitBreakerListener): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Only used for logging purposed for now
| logger=logging.getLogger(__name__) | ||
| # Circuit Breaker Configuration Constants | ||
| MINIMUM_CALLS=20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hardcoded values as do not want user to configure them anywhere in the driver
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Thanks for your contribution! To satisfy the DCO policy in ourcontributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
| {version =">=18.0.0",python =">=3.13",optional=true } | ||
| ] | ||
| pyjwt ="^2.0.0" | ||
| pybreaker ="^1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@vikrantpuppala Can we be sure that adding new library won't break any other client usage?
| pool_connections:Optional[int]=None, | ||
| pool_maxsize:Optional[int]=None, | ||
| user_agent:Optional[str]=None, | ||
| telemetry_circuit_breaker_enabled:Optional[bool]=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should remove it from here and instead set this using a safe flag.@gopalldb what was the rolloutplan for CB in java?
| logger.error("HTTP request failed after retries: %s",e) | ||
| raiseRequestError(f"HTTP request failed:{e}") | ||
| # Try to extract HTTP status code from the MaxRetryError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
setting http_code here which will be used by CircuitBreaker to consider regression in telemetry endpoint
| @dataclass(frozen=True) | ||
| classCircuitBreakerConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Single object to store CB config
| returnbreaker | ||
| @classmethod | ||
| def_create_noop_circuit_breaker(cls)->CircuitBreaker: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In cases where client config setup failed or used by test cases.
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Signed-off-by: Nikhil Suri <nikhil.suri@databricks.com>
Uh oh!
There was an error while loading.Please reload this page.
What type of PR is this?
Description
This PR introduces a circuit breaker pattern to the telemetry system to prevent cascading failures and improve system resilience. The implementation uses the pybreaker library to monitor telemetry request failures and automatically open the circuit when failure rates exceed configurable thresholds, blocking further requests to protect downstream services. When the circuit is open, telemetry requests are temporarily blocked until the system recovers, at which point the circuit transitions to half-open for testing and eventually closes when normal operation resumes. The circuit breaker configuration is centralized with immutable settings, includes comprehensive logging for monitoring, and maintains full backward compatibility with existing telemetry functionality.
How is this tested?
Related Tickets & Documents
https://docs.google.com/document/d/1ftRvby9bwDZzE3s1tOb4hJ4Pd9USiXskb9cDw-uQNPM/edit?usp=sharing