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.

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

Modify BigArrays to take name of circuit breaker #36461

Merged
merged 12 commits into from
Dec 11, 2018

Conversation

Tim-Brooks
Copy link
Contributor

@Tim-Brooks Tim-Brooks commented Dec 11, 2018

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 Tim-Brooks added >bug :Distributed Coordination/Network Http and internode communication implementations :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v7.0.0 v6.6.0 labels Dec 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@Tim-Brooks Tim-Brooks changed the title Modify BigArrays to take single circuit breaker Modify BigArrays to take name of circuit breaker Dec 11, 2018
@Tim-Brooks
Copy link
Contributor Author

I modified this to reflect the fact that you must always access the circuit breaker service due to potential settings changes.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks @tbrooks8, this looks much cleaner to me this way! I'd just like that we keep a single BigArrays constructor.

@@ -380,9 +376,15 @@ public BigArrays(PageCacheRecycler recycler, @Nullable final CircuitBreakerServi

// public for tests
public BigArrays(PageCacheRecycler recycler, @Nullable final CircuitBreakerService breakerService, boolean checkBreaker) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this constructor and make call sites pass CircuitBreaker.REQUEST as a breaker name?

@Tim-Brooks
Copy link
Contributor Author

@jpountz - Thanks.

There is now a public ctor that takes recycler, breaker service and name. And a protected ctor that takes whether we should check breaker (exists for MockBigArrays).

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

LGTM

@Tim-Brooks Tim-Brooks merged commit 790f810 into elastic:master Dec 11, 2018
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Dec 11, 2018
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.
jasontedor added a commit to dnhatn/elasticsearch that referenced this pull request Dec 11, 2018
* elastic/master: (36 commits)
  Add check for minimum required Docker version (elastic#36497)
  Minor search controller changes (elastic#36479)
  Add default methods to DocValueFormat (elastic#36480)
  Fix the mixed cluster REST test explain/11_basic_with_types.
  Modify `BigArrays` to take name of circuit breaker (elastic#36461)
  Move LoggedExec to minimumRuntime source set (elastic#36453)
  added 6.5.4 version
  Add test logging for elastic#35644
  Tests- added helper methods to ESRestTestCase for checking warnings (elastic#36443)
  SQL: move requests' parameters to requests JSON body (elastic#36149)
  [Zen2] Respect the no_master_block setting (elastic#36478)
  Require soft-deletes when access changes snapshot (elastic#36446)
  Switch more tests to zen2 (elastic#36367)
  [Painless] Add extensive tests for def to primitive casts (elastic#36455)
  Add setting to bypass Rollover action (elastic#36235)
  Try running CI against Zulu (elastic#36391)
  [DOCS] Reworked the shard allocation filtering info.  (elastic#36456)
  Log [initial_master_nodes] on formation failure (elastic#36466)
  converting ForbiddenPatternsTask to .java (elastic#36194)
  fixed typo
  ...
Tim-Brooks added a commit that referenced this pull request Dec 11, 2018
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.
@tomcallahan tomcallahan removed the :Distributed Coordination/Network Http and internode communication implementations label Dec 18, 2018
@Tim-Brooks Tim-Brooks deleted the no_singleton_ba branch December 18, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigArrays#newByteArray always uses REQUEST breaker
5 participants