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.

Sign up for GitHub

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

Jump to bottom

Avoid flushing headers when the server returns a single response #9314

Merged
merged 4 commits into from
Oct 9, 2023
Merged

Avoid flushing headers when the server returns a single response #9314

merged 4 commits into from
Oct 9, 2023

Conversation

amirhadadi
Copy link
Contributor

resolves #4884

@sanjaypujare sanjaypujare requested a review from ejona86 June 26, 2022 21:05
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jun 26, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jun 26, 2022
ejona86
ejona86 approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

binder/ also needs updating. You didn't notice it before because -PskipAndroid=true will skip it.

core/src/test/java/io/grpc/internal/AbstractTransportTest.java Outdated Show resolved Hide resolved
@ejona86
Copy link
Member

ejona86 commented Jun 28, 2022

The history has gotten convoluted here and seems broken, and is breaking the diff. Are you fine with me cleaning it up? (Doing so will require force-pushing to the branch, and I'm not entirely sure if I have access to do that but I can certainly try.)

@ejona86 ejona86 self-requested a review June 28, 2022 15:12
ejona86
ejona86 approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I went ahead and cleaned up the history to fix the diff.

@ejona86 ejona86 requested a review from sanjaypujare June 28, 2022 15:16
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 12, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 12, 2022
@larry-safran larry-safran added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Apr 17, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Apr 17, 2023
@larry-safran
Copy link
Contributor

@amirhadadi Would you please complete the EasyCLA and rebase to resolve the conflicts?

@larry-safran larry-safran added the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Sep 28, 2023
@amirhadadi
Copy link
Contributor Author

@larry-safran as we traced the issue of extra segments being sent to flow control PINGs, not sure it's worth merging this PR.
WDYT? Should it be closed?

@ejona86
Copy link
Member

ejona86 commented Oct 2, 2023

With #9847 , we won't be sending PINGs as frequently, especially for small requests/responses on a warmed connection.

Seems this wasn't merged just waiting on @sanjaypujare's review.

@sanjaypujare
Copy link
Contributor

With #9847 , we won't be sending PINGs as frequently, especially for small requests/responses on a warmed connection.

Seems this wasn't merged just waiting on @sanjaypujare's review.

Yes, I noticed that a couple of days ago. Is there still interest in merging this PR? From yours and @amirhadadi 's comments I am guessing this PR may not be needed anymore. Let me know

@ejona86
Copy link
Member

ejona86 commented Oct 2, 2023

My comment was to say this has use, as the thing that made it less useful has been fixed. (And even back when it didn't demonstrate a win, I was approving of it.)

@sanjaypujare
Copy link
Contributor

Okay. @amirhadadi do you want to rebase then I will review. 3 files have conflicts

@sanjaypujare sanjaypujare added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 8, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 8, 2023
sanjaypujare
sanjaypujare approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LGTM

@sanjaypujare sanjaypujare merged commit 8c05455 into grpc:master Oct 9, 2023
6 checks passed
@hutchig hutchig mentioned this pull request Oct 25, 2023
Merged
@ejona86 ejona86 removed the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Nov 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sanjaypujare sanjaypujare sanjaypujare approved these changes

@ejona86 ejona86 ejona86 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid unnecessary flushes for unary responses
5 participants
@amirhadadi @ejona86 @larry-safran @sanjaypujare @grpc-kokoro