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

idle: use LB policy close event as a proxy for channel idleness #6628

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Sep 13, 2023

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

@easwars easwars added this to the 1.59 Release milestone Sep 13, 2023
Copy link
Member

@dfawley dfawley left a 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?

// 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())
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@dfawley dfawley left a 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?

@easwars easwars force-pushed the idle_test_improve_checks branch from 77432c8 to dc8735d Compare September 13, 2023 20:03
@easwars
Copy link
Contributor Author

easwars commented Sep 13, 2023

Do we have existing tests that confirm the LB policy is closed when the channel goes idle?

I thought of the same once I prepared the PR and was going through the diff, before asking for your review :). Added it now.

@dfawley dfawley assigned easwars and unassigned dfawley Sep 13, 2023
@easwars easwars merged commit 9deee9b into grpc:master Sep 13, 2023
@easwars easwars deleted the idle_test_improve_checks branch September 13, 2023 20:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve checks in idleness e2e tests
2 participants