-
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
client: Drop two calls to pretty.ToJSON
and move code outside of lock
#7132
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.
Could you please add a comment to addrConn
's mu
field that says something like "This is taken on the RPC path, so its usage should be minimized as much as possible. TODO: find a lock-free way to retrieve the transport and state from the addrConn without a lock
"
if limit > 5 { | ||
limit = 5 | ||
} | ||
channelz.Infof(logger, ac.channelz, "addrConn: updateAddrs addrs (%d of %d): %v", limit, len(addrs), addrs[:limit]) |
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.
Can you show what this looks like, please? (Mainly I'm not sure: does %v
on a slice call String()
for each member?)
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.
Looks something like this:
addrConn: updateAddrs addrs (3 of 3): [{Addr: "127.0.0.1:35771", ServerName: "", } {Addr: "127.0.0.1:41729", ServerName: "", } {Addr: "127.0.0.1:36587", ServerName: "", }]
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.
Great, thanks!
This may be significant enough to warrant a patch release. We should at least cherry-pick it so it will be in the next patch release, even if we don't want to do a release for only this. |
Done. |
PTAL |
if limit > 5 { | ||
limit = 5 | ||
} | ||
channelz.Infof(logger, ac.channelz, "addrConn: updateAddrs addrs (%d of %d): %v", limit, len(addrs), addrs[:limit]) |
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.
Great, thanks!
clientconn.go
Outdated
// TODO: Find a lock-free way to retrieve the transport and state from the | ||
// addrConn without a lock. |
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.
Oops, "lock-free ... without a lock" - please remove one or the other. Thanks!
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. PTAL
RELEASE NOTES: