-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
idle: use LB policy close event as a proxy for channel idleness #6628
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have existing tests that confirm the LB policy is closed when the channel goes idle?
internal/idle/idle_e2e_test.go
Outdated
// Returns a channel that gets pinged when the balancer is closed. | ||
func registerWrappedRoundRobinPolicy(t *testing.T) chan struct{} { | ||
rrBuilder := balancer.Get(roundrobin.Name) | ||
internal.BalancerUnregister(rrBuilder.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary. Just register the new one over the old one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have existing tests that confirm the LB policy is closed when the channel goes idle?
77432c8
to
dc8735d
Compare
I thought of the same once I prepared the PR and was going through the diff, before asking for your review :). Added it now. |
Existing tests were using the absence of a certain log message in channelz to verify that the channel did not enter IDLE (apart from checking the channel connectivity state). But these checks aren't great because if we decided to change the string we log, then the test case will no longer be valid but will continue to pass.
This PR uses the closing of the LB policy as a proxy for channel entering IDLE mode instead.
Fixes #6612
RELEASE NOTES: none