Skip to content

Conversation

@dapengzhang0
Copy link
Contributor

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
Contributor 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
Contributor 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.

2 participants