Skip to content

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

Merged
srini100 merged 1 commit intov1.14.xfrom
revert-16179-fix_again
Aug 7, 2018
Merged

Revert "Immediately run write closures for failed stream"#16237
srini100 merged 1 commit intov1.14.xfrom
revert-16179-fix_again

Conversation

@ncteisen
Copy link
Copy Markdown
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
Copy Markdown

grpc-testing commented Aug 3, 2018 

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

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
Copy Markdown

grpc-testing commented Aug 3, 2018 

[trickle] No significant performance differences

@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Aug 3, 2018

@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
Copy Markdown

grpc-testing commented Aug 3, 2018 

[microbenchmarks] No significant performance differences

@ncteisen
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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