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

grpclb: Don't go into fallback if we haven't started picking #13339

Merged
merged 1 commit into from
Nov 14, 2017

Conversation

dgquintas
Copy link
Contributor

@dgquintas dgquintas commented Nov 9, 2017

See #13306 for previous discussion.

Fixes #13333

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.1%     +16 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc     +16  +0.1%
      +1.3%     +10 glb_update_locked                                                  +10  +1.3%
      +1.6%      +6 [Unmapped]                                                          +6  +1.6%

 -------------- SHRINKING                                                      --------------
  -0.0%     -16 [None]                                                             -16  -0.0%

  [ = ]       0 TOTAL                                                                0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@markdroth
Copy link
Member

Would be nice to see a test for this. It should be easy to trigger -- just send a resolver update before the fallback timer fires.

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@dgquintas
Copy link
Contributor Author

dgquintas commented Nov 10, 2017

Triggering the pre-existing failure not that easy. As I previously described, we'd need the ability to send resolver updates after the policy creation but before a pick. We could do this in a crude manner by flooding the policy with updates sent from a separate thread.

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Yeah, I see your point -- I was somehow mentally confusing the fallback timer firing and the LB policy starting picking.

In principle, it should probably be possible to test this by letting the channel go into IDLE state and then sending a resolver update. But we don't currently reset started_picking when we go into IDLE (although we probably should; I've added a note about this to our client channel roadmap doc), so there's no way to do this right now.

@dgquintas
Copy link
Contributor Author

Issues: #11008 #13270 #13330 #13385 #13253 #13122

@dgquintas dgquintas merged commit 3810749 into grpc:master Nov 14, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants