Store: If request ctx has an error we do not increment opsFailures counter#3179
Store: If request ctx has an error we do not increment opsFailures counter#3179bwplotka merged 3 commits intothanos-io:masterfrom
Conversation
…unter Signed-off-by: Jarod Watkins <jarod@42lines.net>
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
I believe we need to just skip context.Canceled.
I think timeouts might be a legit failures, no?
pkg/objstore/objstore.go
Outdated
| start := time.Now() | ||
| if err := b.bkt.Delete(ctx, name); err != nil { | ||
| if !b.isOpFailureExpected(err) { | ||
| if !b.isOpFailureExpected(err) && ctx.Err() == nil { |
There was a problem hiding this comment.
| if !b.isOpFailureExpected(err) && ctx.Err() == nil { | |
| if !b.isOpFailureExpected(err) && ctx.Err() != context.Canceled { |
There was a problem hiding this comment.
Agree timeouts are legit failures, but this comment apply to any place, not just here, right? Also, is there a good reason why we don't we the context.Canceled check isOpFailureExpected()?
There was a problem hiding this comment.
From what I have observed, cancelled requests can cause timeouts:
If you notice, query cancels the request but store reports it as aborted. In my opinion that should be reported as cancelled, and not increase the error count.
There was a problem hiding this comment.
or does Aborted != timeout?
There was a problem hiding this comment.
Agree timeouts are legit failures, but this comment apply to any place, not just here, right? Also, is there a good reason why we don't we the
context.CanceledcheckisOpFailureExpected()?
If that is a better place for this then I have no problems making the change.
There was a problem hiding this comment.
isOpFailureExpected is really only for user specified function what to expect. I believe Cancel is valid to be ignored no matter what user specifies, no? (:
There was a problem hiding this comment.
I've updated the PR to use that context.
|
Is there a way for me to view the circleci results without giving them access to my Github account? |
|
You shold have acces to logs, no? https://circleci.com/gh/thanos-io/thanos/11705?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link |
|
When I attempt to visit that URL, I get redirected to login via Github/Bitbucket and it requires read/write access to both public and private repos. I just didn't know if there was a way to view the logs "ready only" or not having to login at all. |
|
Try incognito mode - works just fine (: |
Signed-off-by: Jarod Watkins <jarod@42lines.net>
Signed-off-by: Jarod Watkins <jarod@42lines.net>
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Amazing, thanks!
Some other solution at some point is to have error status as label (:
Good enough for now +1
|
Thank you both! |
3 participants
Signed-off-by: Jarod Watkins jarod@42lines.net
Resolves #3149
Changes
If a request is cancelled or times out we will no longer increment the ops failure counter.