-
Notifications
You must be signed in to change notification settings - Fork 74
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
S3: Return context cancellation from Iter when applicable #62
Conversation
When the context passed to the S3 Iter method is cancelled, the underlying Minio client sometimes immediately closes the results channel and returns without sending the `ctx.Err()` error. Depending on when exactly the context is cancelled, this can result in the `Iter` method not returning any results _and_ not returning an error indicating what happened. Discovered while debugging grafana/mimir#5433 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
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, however, could you please also create an upstream issue in minio-go? It seems like this is something that could be solved there. I also saw this in prod where remote object storage was having problems and sometimes the meta fetcher returned 0 metas with no errors. This is probably the cause. Haven't had time to dig into this myself.
Opened an issue, thanks! |
Specifically to pull in thanos-io/objstore#62 which fixes an issue where all store-gateways unloaded all blocks due to a badly timed shutdown. The library has removed the thanos_ prefix from metrics which our dashboards rely on. I've worked around this change by adding the prefix back. Fixes #5433 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Specifically to pull in thanos-io/objstore#62 which fixes an issue where all store-gateways unloaded all blocks due to a badly timed shutdown. The library has removed the thanos_ prefix from metrics which our dashboards rely on. I've worked around this change by adding the prefix back. Fixes #5433 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Changes
This PR returns
ctx.Err()
from the S3Iter
method instead ofnil
after exhausting theListObjects
channel.When the context passed to the S3 Iter method is cancelled, the underlying Minio client sometimes immediately closes the results channel and returns without sending the
ctx.Err()
error. Depending on when exactly the context is cancelled, this can result in theIter
method not returning any results and not returning an error indicating what happened.Discovered while debugging grafana/mimir#5433
Verification
Tested via Mimir end-to-end test used to reproduce the issue. The S3
Bucket
implementation uses the concrete MinioClient
instead of an interface so writing a unit test would be a challenge without making quite a few changes to theBucket
. I can do that if you'd like but it seemed like a lot of churn for a small change.