Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

Child circuit breakers rely on proper matching of acquire/release pairs.
This can be tricky to get right. If we get it wrong and accidentally
double-release a CB then it may end up with a negative used value.
This is definitely a bad situation in which to find ourselves, but today
in production it's made a whole lot worse because it causes exceptions
on every attempt to report a CircuitBreakerStats or to construct a
parent CircuitBreakingException.

This commit makes the message construction and stats serialization a
little more robust so that it's clearer what is going on in production.

Relates #86059

Child circuit breakers rely on proper matching of acquire/release pairs. This can be tricky to get right. If we get it wrong and accidentally double-release a CB then it may end up with a negative `used` value. This is definitely a bad situation in which to find ourselves, but today in production it's made a whole lot worse because it causes exceptions on every attempt to report a `CircuitBreakerStats` or to construct a parent `CircuitBreakingException`. This commit makes the message construction and stats serialization a little more robust so that it's clearer what is going on in production. Relates elastic#86059
@DaveCTurner DaveCTurner added >bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge v7.17.5 v8.4.0 v8.3.1 labels Jun 21, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jun 21, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @DaveCTurner, I've created a changelog YAML for you.

@DaveCTurner
Copy link
Contributor Author

This is a repeat of #87687 which was reverted due to test failures (failing to account for -1b as a legitimate byte value). See #87687 for reviews.

@elasticsearchmachine elasticsearchmachine merged commit 36d482c into elastic:master Jun 21, 2022
@DaveCTurner DaveCTurner deleted the 2022-06-21-fix-cbe-pr branch June 21, 2022 12:46
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jun 21, 2022
Child circuit breakers rely on proper matching of acquire/release pairs. This can be tricky to get right. If we get it wrong and accidentally double-release a CB then it may end up with a negative `used` value. This is definitely a bad situation in which to find ourselves, but today in production it's made a whole lot worse because it causes exceptions on every attempt to report a `CircuitBreakerStats` or to construct a parent `CircuitBreakingException`. This commit makes the message construction and stats serialization a little more robust so that it's clearer what is going on in production. Relates elastic#86059
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.17 Commit could not be cherrypicked due to conflicts
8.3

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 87881

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jun 21, 2022
Child circuit breakers rely on proper matching of acquire/release pairs. This can be tricky to get right. If we get it wrong and accidentally double-release a CB then it may end up with a negative `used` value. This is definitely a bad situation in which to find ourselves, but today in production it's made a whole lot worse because it causes exceptions on every attempt to report a `CircuitBreakerStats` or to construct a parent `CircuitBreakingException`. This commit makes the message construction and stats serialization a little more robust so that it's clearer what is going on in production. Relates elastic#86059 Backport of elastic#87881
elasticsearchmachine pushed a commit that referenced this pull request Jun 21, 2022
Child circuit breakers rely on proper matching of acquire/release pairs. This can be tricky to get right. If we get it wrong and accidentally double-release a CB then it may end up with a negative `used` value. This is definitely a bad situation in which to find ourselves, but today in production it's made a whole lot worse because it causes exceptions on every attempt to report a `CircuitBreakerStats` or to construct a parent `CircuitBreakingException`. This commit makes the message construction and stats serialization a little more robust so that it's clearer what is going on in production. Relates #86059 Backport of #87881
elasticsearchmachine pushed a commit that referenced this pull request Jun 21, 2022
Child circuit breakers rely on proper matching of acquire/release pairs. This can be tricky to get right. If we get it wrong and accidentally double-release a CB then it may end up with a negative `used` value. This is definitely a bad situation in which to find ourselves, but today in production it's made a whole lot worse because it causes exceptions on every attempt to report a `CircuitBreakerStats` or to construct a parent `CircuitBreakingException`. This commit makes the message construction and stats serialization a little more robust so that it's clearer what is going on in production. Relates #86059
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload Team:Core/Infra Meta label for core/infra team v7.17.5 v8.3.1 v8.4.0

3 participants