v2: Added ability to do context-aware PerformRequest #503
Conversation
utrack
changed the title
|
Bump. Are you OK with this @olivere? |
|
@utrack v2 is rather old, so I'm not pushing too aggressively for that. There has also been some work recently to switch from I won't have time to look into it this week. Maybe next weekend. |
|
Actually, v2 has no context-related code at all atm :) |
|
Yes. The PR mentioned above does exactly that. The However, the services need to be capable of using the |
|
Oh, I see. I'll add support for other services then, thanks. |
|
@utrack Yes, that'd be very helpful. However, keep in mind that I won't be able to merge this week as I'm out of the office. Sorry. |
|
@olivere Bump |
|
@utrack I'm sorry, I was on vacation last week and v2 is not my primary focus, so I updated 5.x first (and work on 6.x already). Please bear with me... it's just a matter of time. Your PR is very welcome, but I want to test it first. |
client.go
Outdated
| if err := c.sniff(c.snifferTimeoutStartup); err != nil { | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) | ||
| defer cancel() | ||
| if err := c.sniff(ctx, c.snifferTimeoutStartup); err != nil { |
There was a problem hiding this comment.
Please remove this and keep the code near to what we have in elastic.v5.
This opens more questions, but does not solve any issues. E.g. why a timeout of 10 seconds? Why cancel the context via timeout and pass a timeout?
client.go
Outdated
| @@ -868,7 +876,7 @@ func (c *Client) healthchecker() { | |||
| // the node state, it marks connections as dead, sets them alive etc. | |||
| // If healthchecks are disabled this is a no-op. | |||
| // The timeout specifies how long to wait for a response from Elasticsearch. | |||
| func (c *Client) healthcheck(timeout time.Duration, force bool) { | |||
| func (c *Client) healthcheck(ctx context.Context, timeout time.Duration, force bool) { | |||
There was a problem hiding this comment.
Same here. Please keep the code as it is in elastic.v5.
|
@utrack Please do not add code that pushes v2 into a different direction than v5. It might be useful, but it's hard enough to keep 3 versions of a codebase. It's even harder when they slightly differ. |
|
Sorry for that, we've got the bug in production just yesterday. Without last two commits when ES cluster goes down - healthcheck waits for response indefinitely, blocking every other hc goroutine, leaking them and breaking the client. @olivere |
|
@utrack Now, that's a bug I'd like to see fixed. But I think we should address that in a separate issue/PR. Once you got the time, can you send a reproducible step-by-step/code snippet? It should also impact v3 and v5 then. |
This reverts commit 5cbeafa.
|
OK @olivere, I'll try to create a reproducible case in a separate PR then :) |
|
Thanks for your support on getting context into v2. Even one of my projects will benefit from this :-) |
Small patch that adds
PerformRequestCfunc which is usualPerformRequestthat passes ctx to the HTTP client.No API breakage or breakage of pre-1.7 builds.