-
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
xds/internal/balancer/outlierdetection: Change string to String #6222
Conversation
Follow up on #6145. |
channelz.Infof(logger, b.channelzParentID, "SuccessRate algorithm detected outlier: %s. Parameters: successRate=%f, mean=%f, stddev=%f, requiredSuccessRate=%f", | ||
addrInfo.string(), successRate, mean, stddev, requiredSuccessRate) | ||
addrInfo, successRate, mean, stddev, requiredSuccessRate) |
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.
I think it would be better to have this on a single line. See: https://g3doc.corp.google.com/go/g3doc/style/decisions.md?cl=head#indentation-confusion. Specifically: https://screenshot.googleplex.com/4R2ZBD6WsCKv6kj.
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.
Also, do we have an idea how this line appears in the log? Like, I don't know if the addrInfo
is readable in the logs.
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.
Ah, this was leftovers from the fact he did it in Java first. Switched.
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.
Logs look like
FailurePercentage algorithm detected outlier: [[{ "Addr": "localhost:60860", "ServerName": "", "Attributes": {}, "BalancerAttributes": {}, "Type": 0, "Metadata": null }]], failurePercentage=100.000000
This looks good, and as you will see in my upcoming wrr_locality PR I changed this String() method on the address (eventually gets called through scw.String() and formatting directive there) to be much more readable all the way down to the Balancer Attributes/Attributes layer.
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.
Nice.
This PR changes string() helper methods to String(), thus implementing a fmt.Stringer() interface and automatically being called by the format package.
RELEASE NOTES: N/A