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.

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

BigArrays#newByteArray always uses REQUEST breaker #31435

Closed
Tim-Brooks opened this issue Jun 19, 2018 · 6 comments · Fixed by #36461
Closed

BigArrays#newByteArray always uses REQUEST breaker #31435

Tim-Brooks opened this issue Jun 19, 2018 · 6 comments · Fixed by #36461
Assignees
@Tim-Brooks
Labels
>bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v7.0.0-beta1

Comments

@Tim-Brooks
Copy link
Contributor

Tim-Brooks commented Jun 19, 2018 •
edited

When you create a byte array using BigArrays, it always increases the
REQUEST breaker. The REQUEST is described as:

/**
 *  The request breaker tracks memory used for particular requests. This
 *  includes allocations for things like the cardinality aggregation, and
 *  accounting for the number of buckets used in an aggregation request.
 *  Generally the amounts added to this breaker are released after a request
 *  is finished.
 */

However, there is also a IN_FLIGHT_REQUESTS breaker. This breaker is
described as:

/**
 * The in-flight request breaker tracks bytes allocated for reading and
 * writing requests on the network layer.
 */

When we serialize a message for writing to a channel in the http or tcp
transports, we use BigArrays which manipulates the REQUEST breaker.
However, based on the descriptions, it seems like this should be the
IN_FLIGHT_REQUESTS breaker.
@Tim-Brooks Tim-Brooks added >bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v7.0.0 labels Jun 19, 2018
@Tim-Brooks Tim-Brooks self-assigned this Jun 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@Tim-Brooks
Copy link
Contributor Author

@s1monw / @danielmitterdorfer - do you agree this is an issue? Daniel, I'm pinging you because in git is looks like you added the IN_FLIGHT_REQUESTS breaker.

@s1monw
Copy link
Contributor

s1monw commented Jun 20, 2018

+1 I agree @dakrone WDYT?

@dakrone
Copy link
Member

dakrone commented Jun 20, 2018

@tbrooks8 the request breaker was added initially before the in flight requests breaker, it was intended for requests that needed to use bigarrays for doing some sort of calculation (like the cardinality aggregation which uses some byte arrays from BigArrays). The in flight breaker is solely for network layer stuff (it could basically be the NETTY_BREAKER)

Personally I think it's nice to have both, since any resources used and released can use the requests breaker, and the in flight request breaker can be solely for network stuff while having different limits. However, I can see how they are close to each other, so if you want to collapse them into the same breaker that'd be fine too (I'd prefer the "REQUESTS" name myself)

@Tim-Brooks
Copy link
Contributor Author

Tim-Brooks commented Jun 20, 2018 •
edited

@dakrone - I'm not really suggesting collapsing them. I was just pointing out that right now REQUESTS is the circuit breaker we use when we claim bytes for serializing a message to write to the network. It seems based on the descriptions that should be IN_FLIGHT_REQUESTS ?

@dakrone
Copy link
Member

dakrone commented Jun 20, 2018

@tbrooks8 yeah if it's possible to change just the one when claiming bytes for serializing network messages, I agree it should be changed to IN_FLIGHT_REQUESTS

@Tim-Brooks Tim-Brooks mentioned this issue Dec 11, 2018
Merged
Tim-Brooks added a commit that referenced this issue Dec 11, 2018
@Tim-Brooks
Modify BigArrays to take name of circuit breaker (#36461)
790f810 
This commit modifies BigArrays to take a circuit breaker name and
the circuit breaking service. The default instance of BigArrays that
is passed around everywhere always uses the request breaker. At the
network level, we want to be using the inflight request breaker. So this
change will allow that.

Additionally, as this change moves away from a single instance of
BigArrays, the class is modified to not be a Releasable anymore.
Releasing big arrays was always dispatching to the PageCacheRecycler,
so this change makes the PageCacheRecycler the class that needs to be
managed and torn-down.

Finally, this commit closes #31435 be making the serialization of
transport messages use the inflight request breaker. With this change,
we no longer push the global BigArrays instnace to the network level.
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this issue Dec 11, 2018
@Tim-Brooks
Modify BigArrays to take name of circuit breaker (elastic#36461)
c6f56b4 
This commit modifies BigArrays to take a circuit breaker name and
the circuit breaking service. The default instance of BigArrays that
is passed around everywhere always uses the request breaker. At the
network level, we want to be using the inflight request breaker. So this
change will allow that.

Additionally, as this change moves away from a single instance of
BigArrays, the class is modified to not be a Releasable anymore.
Releasing big arrays was always dispatching to the PageCacheRecycler,
so this change makes the PageCacheRecycler the class that needs to be
managed and torn-down.

Finally, this commit closes elastic#31435 be making the serialization of
transport messages use the inflight request breaker. With this change,
we no longer push the global BigArrays instnace to the network level.
@Tim-Brooks Tim-Brooks mentioned this issue Dec 11, 2018
Merged
Tim-Brooks added a commit that referenced this issue Dec 11, 2018
@Tim-Brooks
Modify BigArrays to take name of circuit breaker (#36461) (#36501)
a923a1b 
This commit modifies BigArrays to take a circuit breaker name and
the circuit breaking service. The default instance of BigArrays that
is passed around everywhere always uses the request breaker. At the
network level, we want to be using the inflight request breaker. So this
change will allow that.

Additionally, as this change moves away from a single instance of
BigArrays, the class is modified to not be a Releasable anymore.
Releasing big arrays was always dispatching to the PageCacheRecycler,
so this change makes the PageCacheRecycler the class that needs to be
managed and torn-down.

Finally, this commit closes #31435 be making the serialization of
transport messages use the inflight request breaker. With this change,
we no longer push the global BigArrays instnace to the network level.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Tim-Brooks Tim-Brooks

Labels
>bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Modify BigArrays to take name of circuit breaker Tim-Brooks/elasticsearch
5 participants
@dakrone @colings86 @Tim-Brooks @s1monw @elasticmachine