-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
clientv3: clarify retry interceptor logging #10476
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.
The change looks good to me. I find the log message sounds a little wired to me. It is not introduced by your PR, so it does not need to be fixed in this PR:)
clientv3/retry_interceptor.go
Outdated
if lastErr == nil { | ||
return nil | ||
} | ||
logger.Warn( | ||
"retrying unary intercept failed", |
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 understand this function is an interceptor, but the actual function call we are retrying is the invoker. So this message (and other similar messages in this function) sounds a little wired to me. It sounds like some interception failed, which is not what actually happened. Maybe something like "retrying of unary invoker failed" or simply "retrying failed"?
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.
Good point. Updated. PTAL.
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.
lgtm
Log only when it errors + some clarification Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
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.
lgtm
lgtm |
Log only when it errors + some clarification.
FYI. I am debugging around #9949.
And resolve kubernetes/kubernetes#72102.