-
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
objstore: Increase the response header timeout in the S3 provider to 2 minutes and make it configurable #1094
objstore: Increase the response header timeout in the S3 provider to 2 minutes and make it configurable #1094
Conversation
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.
Thank you! Good stuff 🥇
Some minor things:
- Please add an item to changelog.
Also maybe make this configurable, but with 2 minute
default?
And if if you can please add docs 🙇♂️ . What kind of errors did you get and how did you figure you need to bump this? I think this could save time for everyone in the future 👍
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.
Agree with @povilasv
If you make it an option to S3 config you don't need to touch the code if you wish to adjust it someday 👍
This seems very similar to #567, can you explain the difference between the two options? |
@povilasv @bwplotka thank you for your feedback, I'll make the rest of the HTTP options configurable in the same way that it's done in the PR that @midnightconman linked to. |
5db0bce
to
cfab9d9
Compare
@povilasv @bwplotka I think this is ready for another 👀 I've decided to only make that one option configurable in the end as opposed to make every I've refactored the tests to make it a little bit more clear what the default values are for the HTTP client, and reworded some parts of the documentation while I dropped a line about this new option. |
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.
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.
Ran into the same problem ourselves - need to be able to customize this value since sometimes the S3 API just doesn't respond with the headers that fast.
🎉 thank you ❤️ |
Changes
This PR increases the response header timeout in the S3 provider from 15 seconds (introduced in #323) to 2 minutes, an arbitrary number that I've found works well with large blocks as a result of debugging #318. This has solved my recurring problems with the
net/http: timeout awaiting response headers
error.I'm not sure whether a fixed number like this or having a configuration flag is better: while the latter is more flexible I believe it introduces complexity and does not provide much value. I'm happy to update this if that's the preferred solution, though.
Verification
I have been running three different compactor processes against a few buckets that would always fail to be compacted with the aforementioned error, and haven't seen a single error in 4 days.