Skip to content

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`.
@codecov
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.


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 :)!

@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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

For unary methods, received messages are put into buffer from the pool, but buffer is never returned back to pool.

5 participants