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

server: always handle streaming RPCs in separate goroutines to optimize serverWorker availability #7558

Closed
wants to merge 1 commit into from

Conversation

pingkuan
Copy link

Streaming RPCs are typically long-lived and can monopolize the serverWorker for extended periods. To prevent this, streaming RPCs should always be handled in a separate goroutine.

Copy link

linux-foundation-easycla bot commented Aug 24, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Aug 24, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.77%. Comparing base (cfd14ba) to head (c80387a).
Report is 29 commits behind head on master.

Files with missing lines Patch % Lines
server.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7558      +/-   ##
==========================================
- Coverage   81.92%   81.77%   -0.16%     
==========================================
  Files         361      361              
  Lines       27816    27828      +12     
==========================================
- Hits        22789    22756      -33     
- Misses       3838     3869      +31     
- Partials     1189     1203      +14     
Files with missing lines Coverage Δ
server.go 82.04% <76.92%> (-0.19%) ⬇️

... and 24 files with indirect coverage changes

@purnesh42H purnesh42H self-assigned this Aug 26, 2024
@purnesh42H
Copy link
Contributor

@pingkuan thanks for suggestion. I have asked the team for their thoughts on this change

@purnesh42H
Copy link
Contributor

@pingkuan our suggestion would be to use a signal which identifies long live rpcs instead of restricting to streaming RPCs.

@pingkuan
Copy link
Author

pingkuan commented Sep 7, 2024

@purnesh42H Since the package may not be able to determine in advance whether a method will be long-lived, which is typically a decision made by the user during implementation, I suggest offering a server option that allows users to specify which methods should be treated as long-lived and, consequently, should not be handled by the server worker.

@easwars
Copy link
Contributor

easwars commented Sep 16, 2024

Is there an issue open for this? I feel that it would make sense to open an issue, discuss the options we have, finalize one, and then move on to a PR.

@dfawley
Copy link
Member

dfawley commented Sep 16, 2024

Is there an issue open for this? I feel that it would make sense to open an issue, discuss the options we have, finalize one, and then move on to a PR.

+1. E.g. I wonder if we can use a sync.Pool to facilitate worker goroutine re-use?

@dfawley dfawley closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants