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

Recycle pages used by outgoing publications #77407

Conversation

DaveCTurner
Copy link
Contributor

Today PublicationTransportHandler.PublicationContext allocates a bunch
of memory for serialized cluster states and diffs, but it uses a plain
BytesStreamOutput which means that the backing pages are allocated by
the BigArrays#NON_RECYCLING_INSTANCE. With this commit we pass in a
proper BigArrays so that the pages being used can be recycled.

Re-run of #77317

Today `PublicationTransportHandler.PublicationContext` allocates a bunch
of memory for serialized cluster states and diffs, but it uses a plain
`BytesStreamOutput` which means that the backing pages are allocated by
the `BigArrays#NON_RECYCLING_INSTANCE`. With this commit we pass in a
proper `BigArrays` so that the pages being used can be recycled.
@DaveCTurner DaveCTurner added >enhancement :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v8.0.0 labels Sep 8, 2021
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 8, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@DaveCTurner
Copy link
Contributor Author

This failed in CI after merging, see https://gradle-enterprise.elastic.co/s/o2issgehbeciw (despite repeated passes locally, although with a couple of changes since the soak test). I've reverted it and reopened this as a PR to investigate further later.

@DaveCTurner
Copy link
Contributor Author

It turned out to be NBD, one test needed a final stabilise call (DaveCTurner@07af588) and another just had very weak assertions and needed a bit longer to settle down (DaveCTurner@aa14bb1). Otherwise this is what was reviewed in #77317, no need to review the rest again.

@DaveCTurner
Copy link
Contributor Author

Gentle ping for a review @original-brownbear, it's just a couple of small changes over what was already reviewed so should be a quick one.

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM :) sorry this fell of my radar

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 13, 2021
@elasticsearchmachine elasticsearchmachine merged commit 80409d4 into elastic:master Sep 13, 2021
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Sep 13, 2021
Today we protect against releasing a request from a remote node until
its handler has completed, but we do not have the same protection for
requests from the local node. This commit adds the missing refcounting.

Relates elastic#77407
Closes elastic#77634
elasticsearchmachine pushed a commit that referenced this pull request Sep 13, 2021
Today we protect against releasing a request from a remote node until
its handler has completed, but we do not have the same protection for
requests from the local node. This commit adds the missing refcounting.

Relates #77407
Closes #77634
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Sep 13, 2021
Today we protect against releasing a request from a remote node until
its handler has completed, but we do not have the same protection for
requests from the local node. This commit adds the missing refcounting.

Relates elastic#77407
Closes elastic#77634
elasticsearchmachine pushed a commit that referenced this pull request Sep 13, 2021
Today we protect against releasing a request from a remote node until
its handler has completed, but we do not have the same protection for
requests from the local node. This commit adds the missing refcounting.

Relates #77407
Closes #77634
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!) :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >enhancement Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.16.0 v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants