-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use custom roundtripper in the minio client #323
Conversation
This allows us to set a `ResponseHeaderTimeout` which avoids cases where the client blocks forever when the S3 endpoint takes connections but doesn't return anything. Fixes thanos-io#315
pkg/objstore/s3/s3.go
Outdated
IdleConnTimeout: 90 * time.Second, | ||
TLSHandshakeTimeout: 10 * time.Second, | ||
ExpectContinueTimeout: 1 * time.Second, | ||
ResponseHeaderTimeout: 10 * time.Second, |
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.
This transport is identical to the default one from minio except for the ResponseHeaderTimeout
above
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.
First of all: Thanks for helping with this!
Yea, wonder why no one has seen this issue before. I have seen you added issues to minio itself minio/minio-go#979 (please share it next time - I needed to dig minio issues to find it)
I have no way to test it currently. Cannot repro it & don't have s3 environment ready, so I need to trust you with the testing (: By curiosity I would love to test following scenario:
- Change response Header timeout for
Exists
method only (use separate minio I guess for it for testing purposes) - Rerun tests.. will you observe any other issues?
I think I am ok with this, maybe I would increase this timeout a little bit for safety. Basically I would be careful with setting this arbitrary timeout value for all requests (including huge uploads and huge downloads). It sound like requestHeaderTimout does touch only waiting for first part of response (so ~server time + latency and read of all response headers), but there is no much documentation about it.
Having safe timeouts per all different request would the best I think, but not all methods has Context support.
pkg/objstore/s3/s3.go
Outdated
IdleConnTimeout: 90 * time.Second, | ||
TLSHandshakeTimeout: 10 * time.Second, | ||
ExpectContinueTimeout: 1 * time.Second, | ||
ResponseHeaderTimeout: 10 * time.Second, |
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.
Lets please comment what we changed vs default minio and why.
A sorry there, I linked the thanos issue in the minio issue so it appears as reference, but didn't think of explicitly mentioning it in a comment
I guess its pretty uncommon to have an unstable S3 endpoint in general as most ppl use the AWS S3 and not a custom solution and then it will probably rarely happen that the server accepts connections just fine but never returns anything. I still have to dig into what is the actual root cause for this :)
Actually you can test this by opening a port with
Well, behavior seems to be the same which makes sense, as I've updated the timeout to 15 seconds, I think this should be sufficient. But yeah, there seems to be almost no doc around this except for what the |
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.
Actually you can test this by opening a port with nc -l $PORTNUMBER and using that as S3 endpoint for the sidecar
nice (:
OK, let's merge it then.
This allows us to set a
ResponseHeaderTimeout
which avoidscases where the client blocks forever when the S3 endpoint
takes connections but doesn't return anything.
Fixes #315