Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Let the wait_for method accept a callback to support variable interval #18

Closed
wants to merge 2 commits into from

3 participants

@geemus
Owner

Hmm. I wonder if this is the right place for this. The code for wait_for itself is fairly simple and I think does a good job of handling the simple case. I think it could be easy to make an argument that it would be better for us to use backoff in the general case, but I'm not sure we really need to add complexity here. In particular it should be easy enough to skip wait_for altogether and write your own backoff code that checks things on the model I would think. Does that make sense? What do you think?

@mountkin

Hi, @geemus . I think if I write my own wiat_for code in the model, I probably will copy the wait_for code from fog-core and add the callback logical.
Most public IaaS providers' APIs has request rate limits. If we retries too frequently, the rate limits may be hit. On the other hand, if the retry interval is too long, the user experience of the wrapped API may be affected. For example, creating an instance on AWS may take one minute or more, so this code https://github.com/fog/fog/blob/master/lib/fog/aws/models/compute/servers.rb#L106 may call the DescribeInstances interface 60 times or more to wait for the instance to become available. In fact the first 30 retries may be unavailing. If the sleep interval is related to sleep times we can pass a callback like times == 1 ? 20 : 1 to Fog.wait_for to reduce the IaaS API calls.

Recently I'm working on adding QingCloud (https://docs.qingcloud.com/api/index.html , one of the leading IaaS providers in China) API to fog and I found that the wait_for method in fog-core is a bit inconvenient. I think the variable interval feature may be a common requirement, so I opened this PR.

@elight
Collaborator

@mountkin's implementation handles the normal case well, though, by using a default parameter for the "sleeper".

If anyone else needed this behavior, they would wind up implementing it in their own provider–which isn't really any better than copypasta code.

My only request would be to extract the interval calculation into a well-named private method and to call that private method from within wait_for. Determining the interval calculation is a lower level of abstraction than the actual sleep itself.

@mountkin

Hi @elight. Did you mean that implementing it like this? https://gist.github.com/mountkin/9226590

@geemus
Owner

@mountkin - interesting. I kind of like something more along those lines actually. I think it might also be nice if we made the normal interval a proc that could be yielded to as well (it just simplifies to always yield I think). I'm not totally sure what that looks like in the end, but I at least like the idea of having it be separately set-able and always use a proc. What do you think?

@mountkin

@geemus The interval parameter in this approach https://gist.github.com/mountkin/9226590 can be ether a proc or an integer.

But recently I found AWS API promises 'eventual consistence'. So if I create a resource and describe it immediately after the create API returns, it may end up with a xxxNotFound error. Thus our wait_for may be interrupted.
I think we can add another optional parameter to allow users passing more options to wait_for. Or we can provide a new method and pass all the parameters as a Hash.

eg:

# @option options [Array] ignored_errors Ignore errors within this array and resume the retry.
# @option options [Integer] max_retries  Maximum times to retry. Stop retry  ether when times exceeded or timeout occurred.
module Fog
  def self.wait_for(timeout=Fog.timeout, interval=Fog.interval, options={}, &block)
    duration = 0
    start = Time.now
    retries = 0
    max_retries = options[:max_retries] || 999999
    ignored_errors = options[:ignored_errors] || []

    begin
      loop do
        raise Errors::TimeoutError.new("The specified wait_for timeout (#{timeout} seconds) was exceeded") if duration > timeout
        raise if retries > max_retries
        return { :duration => duration } if yield
        sleep(wait_policy(retries += 1, interval))
        duration = Time.now - start
      end
    rescue *ignored_errors => e
      raise if retries > max_retries
      sleep(wait_policy(retries += 1, interval))
      duration = Time.now - start
      retry
    end
  end

  def self.wait_policy(retries, policy)
    interval = policy.respond_to?(:call) ? policy.call(retries) : policy
    interval.to_f
  end
end


r = Fog.wait_for(1, 1, :ignored_errors => [RuntimeError]) do
  raise RuntimeError if rand(2) == 0
  true
end

puts r
@geemus
Owner

Alternatively, perhaps instead of overidding this we should make a new method as you allude to. This new method could always take a proc (instead of making this method more complicated and potentially fragile/confusing to support it). That seems as though it might be a bit cleaner. What do you think?

@mountkin

I also think a new method is better.
I'll try to do it later.

@geemus
Owner

Thanks for helping discuss our way through this, just let me know if you have questions or need help. Thanks!

@mountkin

You're welcome.
I'm not sure how to name the new method. Because the behavior of the new method is similar to the current wait_for. In fact i think it's an enhanced wait_for. Any ideas?

@geemus
Owner

Hmm. I suppose the simplest might be something like wait_for_proc or similar, but that seems a bit confusing (since there is the proc for timing and a proc for the actual logic). So far as that goes I suppose you could even use wait_for as-is and just add extra sleeps, but that is also somewhat awkward. I guess I'm not really sure how best to go from here.

@mountkin

I've changed the wait_for to def self.wait_for(options={}, *bc_params, &block) and added :max_retries, :ignored_errors stuffs.

@geemus
Owner

I guess I'm still reticent to add this level of complexity. I wonder actually if the right (and simpler) solution should be that we work toward replacing the existing wait_for logic with something that uses exponential backoff by default. I've known for some time that this is likely the better approach (and many providers recommend it), but historically this solution worked well enough that we never really bothered to change it. We would need to be careful to make things as backwards compatible as possible and perhaps warn when trying to set intervals or whatever. What do you think?

@mountkin

Hi, @geemus
I'm sorry for the late response.
Indeed my implement increased the complexity of the code. But there's no backward compatible break. Please have a look at this test case https://github.com/mountkin/fog-core/blob/master/tests/core/wait_for_tests.rb#L3

By the way, I realized that the test tool has been switched to rspec. If there are any chances to merge this PR, I can change the test cases to rspec. Otherwise I will move the code to my provider codes and you can close this PR. Thanks.

@geemus
Owner

No problem. I agree that it doesn't break any thing. I was just suggesting that perhaps the easier and better solution would be to simply change wait_for to always use exponential backoff. It is more correct that way and I've considered it before also. It would change how things worked internally, but shouldn't change the interface (so I think we can do it without considering it to be a breaking change). What do you think?

@mountkin

According your suggestions, I've made the changes and opened a new PR #33

@mountkin mountkin closed this
@geemus
Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  lib/fog/core/errors.rb
@@ -19,6 +19,7 @@ class NotFound < Fog::Errors::Error; end
class LoadError < LoadError; end
class TimeoutError< Fog::Errors::Error; end
+ class RetryTimesExceeded< Fog::Errors::Error; end
class NotImplemented < Fog::Errors::Error; end
View
7 lib/fog/core/model.rb
@@ -59,9 +59,12 @@ def symbolize_keys(hash)
end
end
- def wait_for(timeout=Fog.timeout, interval=1, &block)
+ def wait_for(options = {}, *bc_params, &block)
reload_has_succeeded = false
- duration = Fog.wait_for(timeout, interval) do # Note that duration = false if it times out
+
+ options = Fog.wait_for_bc_options(options, *bc_params)
+
+ duration = Fog.wait_for(options) do # Note that duration = false if it times out
if reload
reload_has_succeeded = true
instance_eval(&block)
View
56 lib/fog/core/wait_for.rb
@@ -1,15 +1,53 @@
module Fog
- def self.wait_for(timeout=Fog.timeout, interval=Fog.interval, &block)
+ # Wait for something to change to the specified status.
+ #
+ # @param [Hash] options
+ #
+ # @option options [Integer] :timeout Maximum seconds to wait for the status change.
+ # @option options [Proc] :wait_policy A proc that returns an integer.
+ # The returned value will be used as the parameter of the 'sleep' call.
+ # @option options [Array] :ignored_errors Ignore errors within this array and resume the retry.
+ # @option options [Integer] :max_retries Maximum times to retry.
+ # Stop retry ether when times exceeded or timeout occurred.
+ def self.wait_for(options={}, *bc_params, &block)
duration = 0
- start = Time.now
- until yield || duration > timeout
- sleep(interval.to_f)
- duration = Time.now - start
+ retries = 0
+
+ options = Fog.wait_for_bc_options(options, *bc_params)
+ max_retries = options[:max_retries] || 65535
+ ignored_errors = options[:ignored_errors] || []
+ wait_policy = options[:wait_policy] || Fog.interval
+ timeout = options[:timeout] || Fog.timeout
+
+ begin
+ loop do
+ raise Errors::TimeoutError, "The specified wait_for timeout (#{timeout} seconds) was exceeded" if duration >= timeout
+ raise Errors::RetryTimesExceeded, "The specified :max_retries (#{max_retries}) was exceeded" if retries > max_retries
+ return { :duration => duration } if yield
+ duration += Fog.wait(retries += 1, wait_policy)
+ end
+ rescue *ignored_errors => e
+ raise Errors::RetryTimesExceeded, "The specified :max_retries (#{max_retries}) was exceeded" if retries > max_retries
+ duration += Fog.wait(retries += 1, wait_policy)
+ retry
end
- if duration > timeout
- raise Errors::TimeoutError.new("The specified wait_for timeout (#{timeout} seconds) was exceeded")
- else
- { :duration => duration }
+ end
+
+ def self.wait(retries, policy)
+ interval = policy.respond_to?(:call) ? policy.call(retries) : policy
+ sleep(interval.to_f)
+ end
+
+ def self.wait_for_bc_options(options = {}, *bc_params)
+ # For BC purpose.
+ unless options.kind_of?(Hash)
+ Fog::Logger.deprecation "Calling Fog.wait_for with interger params is deprecated. "\
+ "Please call it this way: Fog.wait_for(:timeout => 3, :wait_policy => proc or interval, :max_retries => 2, :ignored_errors => [some errors])"
+ options = {
+ :timeout => options,
+ :wait_policy => bc_params.first
+ }
end
+ options
end
end
View
1  lib/tasks/test_task.rb
@@ -1,5 +1,6 @@
require "rake"
require "rake/tasklib"
+require "fog/core"
module Fog
module Rake
View
17 tests/core/wait_for_tests.rb
@@ -1,13 +1,28 @@
Shindo.tests('Fog#wait_for', 'core') do
tests("success") do
- tests('Fog#wait_for').formats(:duration => Integer) do
+ tests('Fog#wait_for BC').formats(:duration => Integer) do
Fog.wait_for(1) { true }
end
+
+ tests("Fog#wait_for with :wait_policy").formats(:duration => Integer) do
+ wait_policy = lambda { |times| times * 0.01 }
+ Fog.wait_for(:wait_policy => wait_policy) { true }
+ end
end
tests("failure") do
tests('Fog#wait_for').raises(Fog::Errors::TimeoutError) do
Fog.wait_for(2) { false }
end
+
+ tests("Fog#wait_for with :timeout").raises(Fog::Errors::TimeoutError) do
+ i = 0
+ Fog.wait_for(:timeout => 1) { i += 1; i > 2 }
+ end
+
+ tests("Fog#wait_for with :max_retries").raises(Fog::Errors::RetryTimesExceeded) do
+ i = 0
+ Fog.wait_for(:max_retries => 1) { i += 1; i > 2 }
+ end
end
end
Something went wrong with that request. Please try again.