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

Simplify Netty RefCounting and ByteBuf Consumption #592

Merged
merged 9 commits into from
Apr 24, 2024

Conversation

AndreKurait
Copy link
Member

@AndreKurait AndreKurait commented Apr 16, 2024

Description

Previously, we relied on stream mapping to create the byte buffs with an assumption that no exception would occur in the stream mapping. There was some uncertainty regarding lazy processing of the stream, and overall it was difficult to reason with.

This change seeks to simplify this behavior by introducing a helper method ByteBufUtils::createRefCntNeutralCloseableByteBufStream which constructs a byte buf stream from a collection of byte arrays that keeps track of the reference counted objects it creates and releases them upon the Stream:close which can be leveraged by try-with-resources constructs.

This change also adds in static semantic analysis with google error-prone MustBeClosed

  • Category: Bug fix/Enhancement
  • Why these changes are required? Simplifies tracking of RefCount bugs by streamlining management.
  • What is the old behavior before changes and new behavior after changes? The new behavior is that the HttpByteBufFormatter is much more resilient to RefCount issues.

Issues Resolved

MIGRATIONS-1665

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Unit Testing, deployed to AWS and tested with high throughput to verify no regressions in leaks or ref count issues. (Still seeing some with high load, but not seeing regressions due to this)

Check List

  • [x ] New functionality includes testing
    • [ x] All tests pass, including unit test, integration test and doctest
  • [ x] New functionality has been documented
  • [ x] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@AndreKurait AndreKurait force-pushed the FixForEachRelease branch 2 times, most recently from 537bad1 to e9db598 Compare April 18, 2024 16:02
@AndreKurait AndreKurait changed the title Fix foreach release Simplify Netty RefCounting in HttpByteBufFormatter Apr 18, 2024
@AndreKurait AndreKurait marked this pull request as ready for review April 18, 2024 16:16
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Attention: Patch coverage is 88.31169% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 75.85%. Comparing base (987125c) to head (ae807ff).
Report is 1 commits behind head on main.

Files Patch % Lines
...search/migrations/replay/HttpByteBufFormatter.java 82.14% 4 Missing and 1 partial ⚠️
...h/migrations/replay/ParsedHttpMessagesAsDicts.java 81.81% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #592      +/-   ##
============================================
- Coverage     76.02%   75.85%   -0.17%     
- Complexity     1494     1495       +1     
============================================
  Files           162      165       +3     
  Lines          6348     6362      +14     
  Branches        572      573       +1     
============================================
  Hits           4826     4826              
- Misses         1145     1158      +13     
- Partials        377      378       +1     
Flag Coverage Δ
unittests 75.85% <88.31%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AndreKurait AndreKurait force-pushed the FixForEachRelease branch 3 times, most recently from c9d6bfe to 8ed510f Compare April 18, 2024 20:27
Copy link
Collaborator

@gregschohn gregschohn left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I don't see any blockers, but I'd like to see this again after final cleanup.

@AndreKurait AndreKurait changed the title Simplify Netty RefCounting in HttpByteBufFormatter Simplify Netty RefCounting and ByteBuf Consumption Apr 19, 2024
@AndreKurait AndreKurait force-pushed the FixForEachRelease branch 4 times, most recently from f01cc18 to 2cae6e6 Compare April 19, 2024 20:32
@AndreKurait
Copy link
Member Author

Updated addressing all suggestions from your first review @gregschohn

Signed-off-by: Andre Kurait <[email protected]>
Signed-off-by: Andre Kurait <[email protected]>
@AndreKurait AndreKurait merged commit 9df3614 into opensearch-project:main Apr 24, 2024
6 of 7 checks passed
@AndreKurait AndreKurait deleted the FixForEachRelease branch April 24, 2024 04:03
AndreKurait added a commit to AndreKurait/opensearch-migrations that referenced this pull request Apr 24, 2024
AndreKurait added a commit to AndreKurait/opensearch-migrations that referenced this pull request Apr 25, 2024
AndreKurait added a commit to AndreKurait/opensearch-migrations that referenced this pull request Apr 25, 2024
AndreKurait added a commit to AndreKurait/opensearch-migrations that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants