Increase Dyn DNS timeout to 60, with a retry backoff. #2650

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

bjaspan commented Feb 6, 2014

We've had a rash of Dyn DNS API calls time out recently. In a support ticket with them, they suggested a longer retry period, with less frequent requests after 10 seconds.

This new code runs but has NOT been tested because at the moment their jobs are running fast enough to return status 200, not 307. If the direction of this change is acceptable, I'll do more to ensure it is tested thoroughly.

@bjaspan bjaspan commented on the diff Feb 6, 2014

lib/fog/dynect/dns.rb
job_location = response.headers['Location']
- Fog.wait_for(time_to_wait) do
- response = request(:expects => original_expects, :method => :get, :path => job_location)
- response.body['status'] != 'incomplete'
- end
-
- if response.body['status'] == 'incomplete'
- raise JobIncomplete.new("Job #{response.body['job_id']} is still incomplete")
@bjaspan

bjaspan Feb 6, 2014

It's unrelated to this PR, but old line 132 was unreachable because Fog.wait_for would have already raised an exception.

@bjaspan

bjaspan Feb 6, 2014

We could wrap the Fog.wait_fors and convert TimeoutError into JobIncomplete, of course.

@geemus

geemus Feb 6, 2014

Owner

Good to note, certainly. Seems like it might be better to just remove this? The error from the wait_for itself is presumably good enough?

@bjaspan

bjaspan Feb 6, 2014

Well, it must be good enough so far, since that is what anyone who hits a timeout has always gotten. :-)

@geemus

geemus Feb 7, 2014

Owner

Ha! Good point.

Coverage Status

Coverage remained the same when pulling be94d47 on bjaspan:master into a16a86a on fog:master.

Owner

geemus commented Feb 6, 2014

@bjaspan - sounds like a good, acceptable plan. Fleshing out the tests a bit more sounds good and then hopefully we can get it in for you. Thanks!

bump - been running into this issue a lot lately and was about to draft a similar patch.

Owner

geemus commented Oct 16, 2014

@dalehamel hey, thanks for the bump. The concern that halted us before was that the issue resolved on their end enough that the code wasn't exercised. Have you tried the patch here to see if it solves your issue? I think with some confirmation I'd feel a bit better about bringing it in, instead of it being largely site unseen. Thanks!

Honestly, being able to override the timeout is all that's really needed,
which is a pretty simple change. The problem here is that 10 seconds is
effectively a magic number and isn't tuneable without monkey patching.

I can confirm that manually raising the timeout fixes my issues, and the
job handling stuff added in this patch is just a nice bonus.

On Thursday, October 16, 2014, Wesley Beary notifications@github.com
wrote:

@dalehamel https://github.com/dalehamel hey, thanks for the bump. The
concern that halted us before was that the issue resolved on their end
enough that the code wasn't exercised. Have you tried the patch here to see
if it solves your issue? I think with some confirmation I'd feel a bit
better about bringing it in, instead of it being largely site unseen.
Thanks!


Reply to this email directly or view it on GitHub
#2650 (comment).

Owner

geemus commented Oct 17, 2014

@dalehamel So, to clarify. This is good as-is, we should make it more tunable, we can just add tunability and remove the added complexity of the polling, or other? Thanks.

Yeah I think the best way to go here is to remove the complexity of the
polling - let the app using this api handle a timeout however it wants.

If the timeout could be moved to a config setting (Fog.dyn-timeout?) that
would fix the magic number issue and let apps handle timeouts gracefully,
and / or just wait longer for long requests.

On Friday, October 17, 2014, Wesley Beary notifications@github.com wrote:

@dalehamel https://github.com/dalehamel So, to clarify. This is good
as-is, we should make it more tunable, we can just add tunability and
remove the added complexity of the polling, or other? Thanks.


Reply to this email directly or view it on GitHub
#2650 (comment).

Owner

geemus commented Oct 17, 2014

@dalehamel I guess in terms of config I would lean toward something that you pass in along with credentials when setting up the connection, maybe just dyn_poll_timeout or something. How does that sound? Would you be up for working it in to a pull request? Thanks!

Member

plribeiro3000 commented Dec 23, 2014

Thanks for your efforts, but due to inactivity we are closing this issue to clean things up. Please comment if you have questions or concerns. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment