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

rpc_util: Fix RecvBufferPool deactivation issues #6766

Merged
merged 3 commits into from Feb 23, 2024
Merged

Conversation

hueypark
Copy link
Contributor

@hueypark hueypark commented Nov 5, 2023

Fixes #6578

  • Resolved unintended deactivation scenarios:

    • During unary RPC calls.
    • When compression is enabled.
  • Also corrected missing payInfo.uncompressedBytes in Server.processUnaryRPC.

RELEASE NOTES: none

- Resolved unintended deactivation scenarios:
  - During unary RPC calls.
  - When compression is enabled.

- Also corrected missing `payInfo.uncompressedBytes` in `Server.processUnaryRPC`.
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Merging #6766 (389c89e) into master (70f1a40) will decrease coverage by 6.70%.
Report is 97 commits behind head on master.
The diff coverage is 59.37%.

Additional details and impacted files

@hueypark
Copy link
Contributor Author

hueypark commented Nov 5, 2023

This PR will fix #6578.

@dfawley dfawley self-assigned this Nov 7, 2023
@arvindbr8 arvindbr8 added this to the 1.61 Release milestone Nov 14, 2023
@ginayeh ginayeh requested review from dfawley and zasweq January 3, 2024 16:55
@zasweq zasweq modified the milestones: 1.61 Release, 1.62 Release Jan 23, 2024
@ginayeh ginayeh modified the milestones: 1.62 Release, 1.63 Release Feb 8, 2024
@dfawley dfawley assigned zasweq and unassigned dfawley Feb 16, 2024
@zasweq zasweq changed the title rpu_util: Fix RecvBufferPool deactivation issues rpc_util: Fix RecvBufferPool deactivation issues Feb 16, 2024
Copy link
Contributor

@zasweq zasweq 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 the contribution, overall this LGTM, super minor style nits.

experimental/shared_buffer_pool_test.go Outdated Show resolved Hide resolved
experimental/shared_buffer_pool_test.go Outdated Show resolved Hide resolved
experimental/shared_buffer_pool_test.go Outdated Show resolved Hide resolved
experimental/shared_buffer_pool_test.go Outdated Show resolved Hide resolved

const bufferCount = reqCount * 2 // req + resp
if len(pool.puts) != bufferCount {
t.Fatalf("Expected 10 buffers to be returned to the pool, got %d", len(pool.puts))
Copy link
Contributor

Choose a reason for hiding this comment

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

bufferCount is 20. Should this say 20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and it's exactly twice the reqCount.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but the error message says 10 :). Switch to 20?

Copy link
Contributor

Choose a reason for hiding this comment

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

Last one before merging :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but the error message says 10 :). Switch to 20?

Got it! 389c89e

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks :)!

experimental/shared_buffer_pool_test.go Outdated Show resolved Hide resolved
rpc_util.go Outdated Show resolved Hide resolved
@zasweq zasweq assigned hueypark and unassigned zasweq Feb 21, 2024
@zasweq
Copy link
Contributor

zasweq commented Feb 22, 2024

I'm going to go ahead and approve this, I'm going to be out the next few weeks so once you get to my minor nits one of my teammates can merge this.

@hueypark hueypark removed their assignment Feb 22, 2024
@zasweq zasweq merged commit 5ccf176 into grpc:master Feb 23, 2024
12 of 14 checks passed
arvindbr8 pushed a commit to arvindbr8/grpc-go that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants