Skip to content

Revert "Immediately run write closures for failed stream" #16237

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
Merged
merged 1 commit into from
Aug 7, 2018
Merged

Revert "Immediately run write closures for failed stream" #16237

merged 1 commit into from
Aug 7, 2018

Conversation

ncteisen
Copy link
Contributor

@ncteisen ncteisen commented Aug 3, 2018 •
edited
Loading

This is causing problems with import, and also related to #16211

Reverts #16179

@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE                                                                   FILE SIZE
 ++++++++++++++ GROWING                                                     ++++++++++++++

 -------------- SHRINKING                                                   --------------
  -0.1%     -16 src/core/ext/transport/chttp2/transport/chttp2_transport.cc     -16  -0.1%
      -2.5%     -16 grpc_chttp2_complete_closure_step                               -16  -2.5%

 -+-+-+-+-+-+-+ MIXED                                                       +-+-+-+-+-+-+-
  +0.0%     +16 [None]                                                          -32  -0.0%

  [ = ]       0 TOTAL                                                           -48  -0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

@jtattermusch
Copy link
Contributor

@ncteisen FYI, you are reverting a PR on v1.14.x branch (it was upported to upstream/master via a different PR). Also it looks like this has been released in v1.14.0.

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@ncteisen
Copy link
Contributor Author

ncteisen commented Aug 3, 2018

So what is the right course here? If it is at fault should we revert in .14 then upmerge to master?

@srini100
Copy link
Contributor

srini100 commented Aug 3, 2018

@ncteisen, has #16179 introduced a bug? All tests have passed several times with this change. We just released 1.14.0. If you think this is causing any regression we will have to do a patch soon.

@hcaseyal
Copy link
Contributor

hcaseyal commented Aug 3, 2018

@srini100 @ncteisen @jtattermusch This PR was causing a few internal tests to fail. It should be reverted until we can find out the cause. @mehrdada Can you trigger a new release after this has been reverted?

@hcaseyal hcaseyal self-requested a review August 3, 2018 20:23
@ncteisen ncteisen added the release notes: no Indicates if PR should not be in release notes label Aug 3, 2018
@srini100
Copy link
Contributor

srini100 commented Aug 3, 2018

@hcaseyal, @ncteisen, Please revert this on master first. When sufficient info is available we can decide if a patch release is required.

@ncteisen
Copy link
Contributor Author

ncteisen commented Aug 3, 2018

#16242

I think this will need a patch... it broke tests in google, and it might have caused a performance regression (or a performance regression occurred with coincidentally bad timing)

@hcaseyal
Copy link
Contributor

hcaseyal commented Aug 3, 2018

#16243 will revert this on master

@srini100 srini100 merged commit f3cef38 into v1.14.x Aug 7, 2018
@mehrdada mehrdada deleted the revert-16179-fix_again branch October 21, 2018 06:15
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@hcaseyal hcaseyal hcaseyal approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
release notes: no Indicates if PR should not be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ncteisen @grpc-testing @jtattermusch @srini100 @hcaseyal