-
Notifications
You must be signed in to change notification settings - Fork 65
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
Hedged-request: Handling in HTTP Client Configuration #118
Conversation
Signed-off-by: Vanshikav123 <vanshikav928@gmail.com>
@GiedriusS PTAL! |
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.
Thanks for your work on this 🙇 I added suggestions in line. Please let me know if you have any questions
@@ -135,8 +136,15 @@ func appendHttpOptions(gc Config, opts []option.ClientOption) ([]option.ClientOp | |||
return nil, err | |||
} | |||
|
|||
httpCli := &http.Client{ | |||
hedgedClient, err := hedgedhttp.New(hedgedhttp.Config{ |
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.
All of this hedgedhttp.New()
must be inside Thanos code, not here, because otherwise you will be forcing hedged HTTP requests on all clients of this library.
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 for your assistance ! i want to confirm my query that you want me to create hedgedhttp.Client into the Thanos codebase itself, and expose a configuration option only to enable or disable hedged requests? something like this ?
}) | ||
if err != nil { | ||
return nil, err | ||
} | ||
chain = []credentials.Provider{ | ||
wrapCredentialsProvider(&credentials.EnvAWS{}), | ||
wrapCredentialsProvider(&credentials.FileAWSCredentials{}), | ||
wrapCredentialsProvider(&credentials.IAM{ | ||
Client: &http.Client{ |
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.
The Transport need to come from this function here: https://github.com/thanos-io/objstore/blob/main/client/factory.go#L65
Hey ! @GiedriusS closing this one ! PTAL for #119 |
Changes
Initiation change for
thanos-io/thanos#6712
Verification