Skip to content

Introduce durability of circuit breaking exception #34460

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

Merged
merged 8 commits into from
Nov 2, 2018

Conversation

danielmitterdorfer
Copy link
Member

With this commit we differentiate between permanent circuit breaking exceptions
(which require intervention from an operator and should not be automatically
retried) and transient ones (which may heal themselves eventually and should be
retried). Furthermore, the parent circuit breaker will categorize a circuit
breaking exception as either transient or permanent based on the categorization
of memory usage of its child circuit breakers.

In order to make handling in our client library more consistent with existing
helpers and also help differentiate it from other errors which lead to a node
(temporarily) being removed from the list of nodes to connect to, we will not
use HTTP status code 503 anymore but rather 429.

Closes #31986

With this commit we differentiate between permanent circuit breaking
exceptions (which require intervention from an operator and should not
be automatically retried) and transient ones (which may heal themselves
eventually and should be retried). Furthermore, the parent circuit
breaker will categorize a circuit breaking exception as either transient
or permanent based on the categorization of memory usage of its child
circuit breakers.

Closes elastic#31986
@danielmitterdorfer danielmitterdorfer added >enhancement :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v7.0.0 labels Oct 15, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@danielmitterdorfer
Copy link
Member Author

cc @elastic/es-clients

With this commit we fix a test failure due to an unregistered exception
that we have created for test purposes only. Instead we use now an
already registered class that has the desired properties (only has
numeric fields and overrides `#metadataToXContent()`.
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I left one comment

CircuitBreakingException ex = serialize(new CircuitBreakingException("I hate to say I told you so...", 0, 100));
assertEquals("I hate to say I told you so...", ex.getMessage());
CircuitBreakingException ex = serialize(new CircuitBreakingException("Too large", 0, 100, CircuitBreaker.Durability.TRANSIENT),
randomFrom(Version.V_7_0_0_alpha1));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the randomFrom for a single version here? Especially since this isn't planned for backporting this can be Version.CURRENT right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I've removed the randomization now.

@danielmitterdorfer danielmitterdorfer merged commit ccbe80c into elastic:master Nov 2, 2018
@danielmitterdorfer
Copy link
Member Author

Thanks for your review @dakrone. I've also marked it as breaking now as we change the HTTP response status code.

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload >enhancement v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants