search

fanatic / go-infoblox

Golang Infoblox WAPI Library (Deprecated)
16 stars 26 forks source link

switch to using pester.Client instead of http.Client #29

Open jrandall opened 6 years ago

jrandall commented 6 years ago

switches from using standard http.Client to pester.Client in order to add retries with backoff in case the infoblox server responds with a temporary error or the connection is interrupted.

fanatic commented 6 years ago

I have to think about this. I'm not sure it's common for API client libraries to implement retries internally, especially given the nature of Infoblox.

I wouldn't want to send a request for a new Host record, have Infoblox create one but send an error response, and accidentally create multiple Hosts.

It also doesn't keep track of non-retryable errors. The documentation states..

Server Error. The error was not caused by any error in the request. Depending on the error the request may be successfully repeated as is.

I'd be concerned about retrying every returned 5xx status code without regard to the error message itself.

In our production use of this library, we haven't found Infoblox connections to be intermittent or for our appliance to return temporary errors. It's generally a stable and well behaving product.

jrandall commented 6 years ago

We have had a lot of trouble with connectivity to our infoblox, although it seems that may be due to it intentionally throttling connections when making concurrent connections from the same client machine. We are using go-infoblox from terraform-provider-infoblox, and terraform does 10 operations in parallel by default, and in that configuration we would get a lot of failures from go-infoblox because the server would not even answer the TCP request for >60s in some cases.

The change to pester worked around the problem, but at the expense of very long delays. We have since implemented a better fix which is to limit terraform-provider-infoblox to a single request to infoblox at a time (using a mutex), which seems to eliminate the need for the pester fix.

On Thu, Nov 30, 2017 at 10:56 PM, Jason Parrott notifications@github.com wrote:

I have to think about this. I'm not sure it's common for API client libraries to implement retries internally, especially given the nature of Infoblox.

I wouldn't want to send a request for a new Host record, have Infoblox create one but send an error response, and accidentally create multiple Hosts.

It also doesn't keep track of non-retryable errors. The documentation states..

Server Error. The error was not caused by any error in the request. Depending on the error the request may be successfully repeated as is.

I'd be concerned about retrying every returned 5xx status code without regard to the error message itself.

In our production use of this library, we haven't found Infoblox connections to be intermittent or for our appliance to return temporary errors. It's generally a stable and well behaving product.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fanatic/go-infoblox/pull/29#issuecomment-348347843, or mute the thread https://github.com/notifications/unsubscribe-auth/AAb-eXaSOL652qODomSL_A3c4KJdACplks5s7zJ_gaJpZM4QwhX5 .