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: should not ignore subchannels with CONNECTING state in aggregating the overall LB state #7959

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Mar 11, 2021

When aggregating the overall load balancing state, we should treat both IDLE and CONNECTING subchannels as "connection in progress". Otherwise, RPCs could fail prematurely if one subchannel enters TF while all the others are still in CONNECTING.

#7816 made each individual subchannel stay in TF until READY if it previously was in TF. So subchannels with CONNECTING state are those in first time connecting. We should give them time to connect.

See more comment in #7816 (comment).

@voidzcy voidzcy requested a review from ejona86 March 11, 2021 23:47
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.

The commit message is lacking comparing to the PR description. Please include the full PR description in the commit message. Ideally you'd also mention the commit hash of Kun's commit that changed the behavior to make this appropriate.

@voidzcy voidzcy force-pushed the bugfix/grpclb_should_not_ignore_connecting_subchannel branch from c019a6c to ede7e9a Compare March 12, 2021 00:12
…g the overall load balancing state.

We should treat both IDLE and CONNECTING subchannels as "connection in progress" when aggregating for the overall load balancing state. Otherwise, RPCs could fail prematurely if one subchannel enters TF while all the others are still in CONNECTING.

23d2796 made each individual subchannel stay in TF until READY if it previously was in TF. So subchannels with CONNECTING state are those in first time connecting. We should give them time to connect.
@voidzcy voidzcy force-pushed the bugfix/grpclb_should_not_ignore_connecting_subchannel branch from ede7e9a to 29de6df Compare March 12, 2021 00:14
@voidzcy voidzcy merged commit 9c562c8 into grpc:master Mar 12, 2021
@zhangkun83
Copy link
Contributor

Thanks for spotting and fixing it!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants