Skip to content
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

Improve flaky endpoint handling #205

Merged
merged 9 commits into from Jan 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions .travis.yml
Expand Up @@ -4,8 +4,6 @@ rvm:
- 2.3.0
- 2.2.5
- 2.1.9
- 2.0.0
- 1.9.3
- ruby-head
- jruby
before_script:
Expand All @@ -20,5 +18,4 @@ matrix:
allow_failures:
- rvm: ruby-head
- rvm: jruby
- rvm: 2.0.0
fast_finish: true
7 changes: 7 additions & 0 deletions CHANGELOG.md
@@ -1,3 +1,10 @@
### 0.11.0 / NOT RELEASED YET

* fix: fix retry logging
* feature: allow retries to be configured for all query types in client settings
* feature: allow configrable wait time between retries
* feature: detect errors as error messages in a response body delivered with HTTP 200

### 0.10.1 / 2016-05-04

* fix: handle invalid codepoints in character references
Expand Down
2 changes: 1 addition & 1 deletion lib/rets.rb
Expand Up @@ -3,7 +3,7 @@
require 'nokogiri'

module Rets
VERSION = '0.10.1'
VERSION = '0.11.0'

HttpError = Class.new(StandardError)
MalformedResponse = Class.new(ArgumentError)
Expand Down
19 changes: 16 additions & 3 deletions lib/rets/client.rb
Expand Up @@ -105,9 +105,11 @@ def find_with_given_retry(retries, opts)
end

def handle_find_failure(retries, opts, e)
if retries < opts.fetch(:max_retries, 3)
max_retries = fetch_max_retries(opts)
if retries < max_retries
retries += 1
client_progress.find_with_retries_failed_a_retry(e, retries)
wait_before_next_request
client_progress.find_with_retries_failed_a_retry(e, retries, max_retries)
clean_setup
find_with_given_retry(retries, opts)
else
Expand All @@ -116,14 +118,25 @@ def handle_find_failure(retries, opts, e)
end
end

def fetch_max_retries(hash)
hash[:max_retries] || options.fetch(:max_retries, 3)
end

def wait_before_next_request
sleep_time = Float(options.fetch(:recoverable_error_wait_secs, 0))
if sleep_time > 0
logger.info "Waiting #{sleep_time} seconds before next attempt"
sleep sleep_time
end
end

def find_every(opts)
raise ArgumentError.new("missing option :search_type (provide the name of a RETS resource)") unless opts[:search_type]
raise ArgumentError.new("missing option :class (provide the name of a RETS class)") unless opts[:class]

params = {
"SearchType" => opts.fetch(:search_type),
"Class" => opts.fetch(:class),

"Count" => opts[:count],
"Format" => opts.fetch(:format, "COMPACT"),
"Limit" => opts[:limit],
Expand Down
4 changes: 2 additions & 2 deletions lib/rets/client_progress_reporter.rb
Expand Up @@ -18,10 +18,10 @@ def initialize(logger, stats, stats_prefix)
@stats_prefix = stats_prefix
end

def find_with_retries_failed_a_retry(exception, retries)
def find_with_retries_failed_a_retry(exception, retries, max_retries)
@stats.count("#{@stats_prefix}find_with_retries_failed_retry")
@logger.warn("Rets::Client: Failed with message: #{exception.message}")
@logger.info("Rets::Client: Retry #{retries}/3")
@logger.info("Rets::Client: Retry #{retries}/#{max_retries}")
end

def find_with_retries_exceeded_retry_count(exception)
Expand Down
8 changes: 4 additions & 4 deletions rets.gemspec
@@ -1,15 +1,15 @@
# -*- encoding: utf-8 -*-
# stub: rets 0.10.1.20160503205231 ruby lib
# stub: rets 0.11.0.20170130173054 ruby lib

Gem::Specification.new do |s|
s.name = "rets"
s.version = "0.10.1.20160503205231"
s.version = "0.11.0.20170130173054"

s.required_rubygems_version = Gem::Requirement.new(">= 0") if s.respond_to? :required_rubygems_version=
s.require_paths = ["lib"]
s.authors = ["Estately, Inc. Open Source"]
s.date = "2016-05-03"
s.description = "[![Build Status](https://secure.travis-ci.org/estately/rets.png?branch=master)](http://travis-ci.org/estately/rets)\nA pure-ruby library for fetching data from [RETS] servers.\n\n[RETS]: http://www.rets.org"
s.date = "2017-01-30"
s.description = "[![Build Status](https://secure.travis-ci.org/estately/rets.png?branch=master)](http://travis-ci.org/estately/rets)\nA pure-ruby library for fetching data from [RETS] servers.\n\nIf you're looking for a slick CLI interface check out [retscli](https://github.com/summera/retscli), which is an awesome tool for exploring metadata or learning about RETS.\n\n[RETS]: http://www.rets.org"
s.email = ["opensource@estately.com"]
s.executables = ["rets"]
s.extra_rdoc_files = ["CHANGELOG.md", "Manifest.txt", "README.md"]
Expand Down
7 changes: 7 additions & 0 deletions test/test_client.rb
Expand Up @@ -176,6 +176,13 @@ def test_find_retries_on_errors
@client.find(:all, :foo => :bar)
end

def test_find_waits_configured_time_before_next_request
@client.options[:recoverable_error_wait_secs] = 3.14
@client.expects(:sleep).with(3.14).times(3)
@client.stubs(:find_every).raises(Rets::MiscellaneousSearchError.new(0, 'Foo'))
@client.find(:all, :foo => :bar) rescue nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a little funny about this rescue nil. I assume this line raises an exception, can we wrap this line in a expect {...}.to raise_error instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts here were that asserting on the raised error might make it less apparent that the test was about the sleep rather than the exception being raised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense... I have a long cultivated aversion to rescue nil, but I suppose it makes sense in this context.

end

def test_find_eventually_reraises_errors
@client.stubs(:find_every).raises(Rets::AuthorizationFailure.new(401, 'Not Authorized'))
@client.stubs(:login)
Expand Down