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

xds: Do not failover priority when IDLE->CONNECTING #8926

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

dapengzhang0
Copy link
Member

Consists of two commits, the fix is in 2nd one, and the 1st only improves the test and does not impact the fix even without it.

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 first commit with the test changes looks fine. But the second one looks broken. It doesn't propagate the picker change up the tree, so the channel will continue using the old picker. READY→CONNECTING will thus return an old transport for potentially a long time.

@dapengzhang0
Copy link
Member Author

The first commit with the test changes looks fine. But the second one looks broken. It doesn't propagate the picker change up the tree, so the channel will continue using the old picker. READY→CONNECTING will thus return an old transport for potentially a long time.

That does break. Redone the fix.

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.

I need to stare at this new version a bit longer, but I'll mention that I thought the earlier version would have worked just by adding a call to tryNextPriority. Since the subpolicy state wasn't changed, we'd expect the tryNextPolicy to be equivalent to the last time it was called, except it will pick up the new picker.

@dapengzhang0
Copy link
Member Author

I thought the earlier version would have worked just by adding a call to tryNextPriority.

That would also work, but I really don't like to update the overall state like (READY, BUFFER_PICKER) in which READY is the previous child state and BUFFER_PICKER is the latest child picker

@ejona86 ejona86 merged commit 89e53dc into grpc:master Feb 24, 2022
@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Feb 24, 2022
@dapengzhang0 dapengzhang0 deleted the fix-priority branch February 25, 2022 00:49
@ejona86 ejona86 removed the TODO:backport PR needs to be backported. Removed after backport complete label Mar 3, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2022
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

2 participants