Skip to content

Fix RecyclerBytesStreamOutput corrupting when ending write on page boundary #95114

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

Conversation

original-brownbear
Copy link
Member

Fixes the fact that we don't put the current page offset to zero when we already have capacity in the buffer but are not on the last page because of a seek.
Also removes unused reset() method since it added one needless place for me to check when trying to reason this out.

…undary

Fixes the fact that we don't put the current page offset to zero when we
already have capacity in the buffer but are not on the last page because
of a seek.
@original-brownbear original-brownbear added >bug :Distributed Coordination/Network Http and internode communication implementations v8.8.0 labels Apr 10, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @original-brownbear, I've created a changelog YAML for you.

@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Apr 10, 2023
@elasticsearchmachine
Copy link
Collaborator

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

currentCapacity += pageSize;
}
if (currentPageOffset == pageSize) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also faster for the rare case where we have to allocate multiple pages in a loop to make space here.

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM but would this have been caught by an existing test (maybe testSeek) eventually? If not I think we need some more evil randomised tests in this area.

@original-brownbear
Copy link
Member Author

Thanks David!

If not I think we need some more evil randomised tests in this area.

Incoming today, I found another bug with this thing I believe and will add some more testing to that PR.

@original-brownbear original-brownbear merged commit 2277fbe into elastic:main Apr 11, 2023
@original-brownbear original-brownbear deleted the fix-write-to-page-boundary-exception branch April 11, 2023 06:24
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request May 24, 2023
…undary (elastic#95114)

Fixes the fact that we don't put the current page offset to zero when we
already have capacity in the buffer but are not on the last page because
of a seek.
elasticsearchmachine pushed a commit that referenced this pull request May 24, 2023
…undary (#95114) (#96322)

Fixes the fact that we don't put the current page offset to zero when we
already have capacity in the buffer but are not on the last page because
of a seek.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 31, 2023
DaveCTurner added a commit that referenced this pull request May 31, 2023
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 31, 2023
pquentin pushed a commit to pquentin/elasticsearch that referenced this pull request May 31, 2023
pquentin added a commit that referenced this pull request May 31, 2023
* [DOCS] Forward port 8.7.1 release notes

* Add known issue docs for #95114 (#96448)

---------

Co-authored-by: David Turner <[email protected]>
arteam added a commit to arteam/elasticsearch that referenced this pull request Nov 3, 2023
The recycled buffer issued was fixed in elastic#95114, so we can unmute the test

See elastic#95096
arteam added a commit that referenced this pull request Nov 6, 2023
The recycled buffer issued was fixed in #95114, so we can unmute the test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Network Http and internode communication implementations Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants