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

clientv3: clarify retry interceptor logging #10476

Merged
merged 2 commits into from
Feb 15, 2019
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Feb 14, 2019

Log only when it errors + some clarification.

FYI. I am debugging around #9949.
And resolve kubernetes/kubernetes#72102.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 14, 2019

/cc @jpbetz @xiang90 @hexfusion

Copy link
Contributor

@jingyih jingyih left a 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:)

if lastErr == nil {
return nil
}
logger.Warn(
"retrying unary intercept failed",
Copy link
Contributor

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Updated. PTAL.

Copy link
Contributor

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 <leegyuho@amazon.com>
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

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

lgtm

@jpbetz
Copy link
Contributor

jpbetz commented Feb 15, 2019

lgtm

@gyuho gyuho merged commit 784daa0 into etcd-io:master Feb 15, 2019
@gyuho gyuho deleted the client-log branch February 15, 2019 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

kube-apiserver 1.13.x refuses to work when first etcd-server is not available.
4 participants