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.

Sign up for GitHub

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

Jump to bottom

balancergroup: improve observability around balancer cache behavior #6597

Merged
merged 3 commits into from
Aug 31, 2023
Merged

balancergroup: improve observability around balancer cache behavior #6597

merged 3 commits into from
Aug 31, 2023

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Aug 29, 2023

RELEASE NOTES: none

@easwars easwars requested a review from dfawley August 29, 2023 21:21
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Aug 29, 2023
@easwars easwars added this to the 1.58 Release milestone Aug 29, 2023
dfawley
dfawley reviewed Aug 29, 2023
View reviewed changes
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.

Should these be at a higher verbosity? I don't think we want to log these by default for everyone, do we?

@easwars
Copy link
Contributor Author

easwars commented Aug 30, 2023

Should these be at a higher verbosity? I don't think we want to log these by default for everyone, do we?

Fair point. Moved them under V2 and also fixed one outlier log which had a balancer group prefix. We are already using a prefix logger here.

dfawley
dfawley reviewed Aug 31, 2023
View reviewed changes
internal/balancergroup/balancergroup.go Outdated
@@ -403,7 +408,7 @@ func (bg *BalancerGroup) Remove(id string) {

sbToRemove, ok := bg.idToBalancerConfig[id]
if !ok {
bg.logger.Infof("balancer group: trying to remove a non-existing locality from balancer group: %v", id)
bg.logger.Warningf("Child policy for locality %q does not exist in the balancer group", id)
Copy link
Member

Choose a reason for hiding this comment

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

Should this even be Error so our tests fail? I.e. does it imply a coding error on our part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ... Making Close() or Remove() idempotent does seem like a good thing to have. But it could point to a coding error on the user of the balancer group, like why are they having a reference to a closed locality. I tried changing it to error and tests are still passing. So, I think we can have it that way, and if at some point, if we run into a case where this can legitimately happen, we can re-evaluate. Thanks.

dfawley
dfawley approved these changes Aug 31, 2023
View reviewed changes
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.

Approving but please see the inline question.

@dfawley dfawley assigned easwars and unassigned dfawley Aug 31, 2023
@easwars easwars merged commit 778e638 into grpc:master Aug 31, 2023
1 check passed
@easwars easwars deleted the balancergroup_lofs branch August 31, 2023 18:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@easwars easwars

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.58 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@easwars @dfawley