Skip to content

Fix for issue #15458#15505

Merged
yashykt merged 4 commits intogrpc:masterfrom
yashykt:15458fix
May 23, 2018
Merged

Fix for issue #15458#15505
yashykt merged 4 commits intogrpc:masterfrom
yashykt:15458fix

Conversation

@yashykt
Copy link
Copy Markdown
Member

@yashykt yashykt commented May 22, 2018 •
edited
Loading

Fixes #15458

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

[microbenchmarks] No significant performance differences

@hcaseyal hcaseyal self-requested a review May 22, 2018 20:09
@yashykt yashykt requested review from dgquintas and jtattermusch May 22, 2018 22:37
dgquintas
dgquintas approved these changes May 22, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgquintas dgquintas left a comment

Choose a reason for hiding this comment

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

Can you add the content of what you commented at #15458 (comment) inside comments at the appropriate places in the code? It makes following the intent of the test much easier.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 22, 2018 

[trickle] No significant performance differences

@dgquintas
Copy link
Copy Markdown
Contributor

dgquintas commented May 22, 2018

Please wait for @sreecha 's feedback before merging.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

[microbenchmarks] No significant performance differences

@yashykt
Copy link
Copy Markdown
Member Author

yashykt commented May 23, 2018

Thanks for taking a looking @dgquintas. I'll wait for @sreecha 's review as well :)

sreecha
sreecha approved these changes May 23, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@sreecha sreecha left a comment

Choose a reason for hiding this comment

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

Talked to Yash offline and asked him to add a comment to explain that "5000" magic number in the deadline

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

[microbenchmarks] No significant performance differences

@yashykt
Copy link
Copy Markdown
Member Author

yashykt commented May 23, 2018

All green!
Thanks for reviewing!

@yashykt yashykt merged commit b80487e into grpc:master May 23, 2018
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented May 23, 2018

Thanks @yashykt for the fix!

@yashykt yashykt added the release notes: no Indicates if PR should not be in release notes label Jul 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 17, 2018
@lock lock bot unassigned sreecha Oct 17, 2018
@yashykt yashykt deleted the 15458fix branch October 26, 2018 20:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hcaseyal hcaseyal Awaiting requested review from hcaseyal

@jtattermusch jtattermusch Awaiting requested review from jtattermusch

2 more reviewers

@dgquintas dgquintas dgquintas approved these changes

@sreecha sreecha sreecha 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

@yashykt @grpc-testing @dgquintas @jtattermusch @sreecha