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

fix: return last err from processClient #37

Merged
merged 1 commit into from
Oct 17, 2021
Merged

fix: return last err from processClient #37

merged 1 commit into from
Oct 17, 2021

Conversation

xonvanetta
Copy link
Contributor

It didn't say why it failed just that it reach max attempts.
This was used to debug a error in coredns.

This is the issue we found out after we noticed that it was working
without cache pluigin.
coredns/coredns#4735

@denis-tingaikin
Copy link
Member

I think this patch makes sense.

Could you please look at failed job https://github.com/networkservicemesh/fanout/pull/37/checks?check_run_id=3364249178?

@xonvanetta
Copy link
Contributor Author

Sure can do, but in a short while, not currently at home.

Also could we do something like attempt limit has been reached, last err: %w?

It didn't say why it failed just that it reach max attempts.
This was used to debug a error in coredns.

This is the issue we found out after we noticed that it was working
without cache pluigin.
coredns/coredns#4735

Signed-off-by: Vanetta <11271952+xonvanetta@users.noreply.github.com>
@xonvanetta
Copy link
Contributor Author

@denis-tingaikin bump

if err == nil {
return &response{client: c, response: msg, start: start, err: err}
}
if f.attempts != 0 {
j++
}
}
return &response{client: c, response: nil, start: start, err: errors.New("attempt limit has been reached")}
return &response{client: c, response: nil, start: start, err: errors.Wrapf(err, "attempt limit has been reached, last err")}
Copy link
Member

Choose a reason for hiding this comment

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

Inner error message will be also available:
https://play.golang.org/p/xYkR4ZaMf58

Suggested change
return &response{client: c, response: nil, start: start, err: errors.Wrapf(err, "attempt limit has been reached, last err")}
return &response{client: c, response: nil, start: start, err: errors.Wrapf(err, "attempt limit has been reached")}

@Funami580 Funami580 mentioned this pull request Oct 15, 2021
@denis-tingaikin denis-tingaikin merged commit 818e324 into networkservicemesh:master Oct 17, 2021
@denis-tingaikin
Copy link
Member

denis-tingaikin commented Oct 17, 2021

@xonvanetta Merged this to help @Funami580 catch the problem with TLS.

Be free to resolve comment in separate PR.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants