Skip to content
This repository was archived by the owner on Oct 5, 2021. It is now read-only.

Swap out pester for Retriable; Retry github errors #75

Closed
wants to merge 5 commits into from
Closed

Swap out pester for Retriable; Retry github errors #75

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
757d77f
Swap out pester for Retriable; Retry github errors
Nov 15, 2017
72a9aad
Update travis config
Nov 15, 2017
c33a6e7
Travis config
Nov 15, 2017
10bc362
Travis config
Nov 15, 2017
c42e60b
Use an on_retry Proc to always raise certain S3 exceptions
Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ cache:
- bundler
- apt
rvm:
- ruby-2.1.6
- 2.1.6
- 2.2.8
- 2.3.5
- 2.4.2

addons:
postgresql: "9.2"
Expand All @@ -29,6 +32,10 @@ env:
- TEST_SUITE="rspec spec --color --format documentation"
- TEST_SUITE="rake karma:run"

matrix:
allow_failures:
- rvm: 2.4.2

before_install:
- "export DISPLAY=:99.0"
- "sh -e /etc/init.d/xvfb start"
Expand Down
3 changes: 2 additions & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ gem 'foreman'
gem 'haml'
gem 'hyperresource'
gem 'jquery-rails'
gem 'json', '>= 1.8.5'
gem 'omniauth-google-oauth2'
gem 'omniauth-saml', '~> 1.4.1'
gem 'paper_trail', '~> 4.0.0'
gem 'pester', '~> 1.0'
gem 'pg'
gem 'pg_stream', '~> 0.1.0'
gem 'puma', '2.11.3'
Expand All @@ -34,6 +34,7 @@ gem 'responders', '~> 2.0'
gem 'resque'
gem 'resque-pool', '~> 0.5.0'
gem 'resque-web', '0.0.6', require: 'resque_web'
gem 'retriable', '~> 3.1'
gem 'roar'
gem 'rollbar', '~> 2.3.0'
gem 'sass-rails'
Expand Down
9 changes: 5 additions & 4 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ GEM
thor (>= 0.14, < 2.0)
jquery-ui-rails (5.0.5)
railties (>= 3.2.16)
json (1.8.3)
json (1.8.6)
jwt (1.5.1)
kaminari (0.16.3)
actionpack (>= 3.0.0)
Expand Down Expand Up @@ -214,7 +214,6 @@ GEM
activerecord (>= 3.0, < 6.0)
activesupport (>= 3.0, < 6.0)
request_store (~> 1.1)
pester (1.0.0)
pg (0.18.3)
pg_stream (0.1.0)
pg (>= 0.18.2)
Expand Down Expand Up @@ -317,6 +316,7 @@ GEM
http-cookie (>= 1.0.2, < 2.0)
mime-types (>= 1.16, < 3.0)
netrc (~> 0.7)
retriable (3.1.1)
roar (1.0.3)
representable (>= 2.0.1, <= 3.0.0)
rollbar (2.3.0)
Expand Down Expand Up @@ -450,11 +450,11 @@ DEPENDENCIES
haml
hyperresource
jquery-rails
json (>= 1.8.5)
launchy
omniauth-google-oauth2
omniauth-saml (~> 1.4.1)
paper_trail (~> 4.0.0)
pester (~> 1.0)
pg
pg_stream (~> 0.1.0)
poltergeist
Expand All @@ -470,6 +470,7 @@ DEPENDENCIES
resque
resque-pool (~> 0.5.0)
resque-web (= 0.0.6)
retriable (~> 3.1)
roar
rollbar (~> 2.3.0)
rspec-rails
Expand All @@ -486,4 +487,4 @@ DEPENDENCIES
zeroclipboard-rails (= 0.1.0)

BUNDLED WITH
1.11.2
1.15.1
20 changes: 8 additions & 12 deletions config/initializers/pester.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
Pester.configure do |c|
c.logger = Rails.logger

c.environments[:schema_refresh] = {
delay_interval: 3,
max_attempts: 15,
on_retry: Pester::Behaviors::Sleep::Linear
Retriable.configure do |c|
c.contexts[:schema_refresh] = {
base_interval: 3,
tries: 15,
}

c.environments[:s3] = {
delay_interval: 30.seconds,
max_attempts: 10,
on_retry: Pester::Behaviors::Sleep::Constant,
reraise_error_classes: [Aws::S3::Errors::NoSuchKey, Aws::S3::Errors::NoSuchBucket]
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm what about these reraise classes?

Copy link
Author

@apurvis apurvis Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

there's no blacklist in Retriable, so you'd either have to explicitly specify via whitelist what to retry, or i guess possibly wait for this PR: kamui/retriable#55 that would support an arbitrary Proc evaluation which could do the black/whitelisting.

what's the reason for these exceptions not being retried? is it still even a valid concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think these are all unrecoverable errors. ie, retrying isn't going to help if you are missing keys or your bucket config is wrong

Copy link
Author

Choose a reason for hiding this comment

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

oh i think this got put in because it was annoying waiting for it to retry when you have the AWS keys misconfigured. i'll put in a tweak to replicate this behavior.

c.contexts[:s3] = {
base_interval: 30,
multiplier: 1,
tries: 10
}
end
16 changes: 9 additions & 7 deletions lib/csv_helper/aws.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,20 @@ class Aws < Base
S3_FOLDER = APP_CONFIG['s3_folder'] || 'results'

def url
Pester.s3.retry do
Retriable.with_context(:s3) do
obj = AwsS3.resource.bucket(S3_BUCKET).object(key)
if obj.exists?
obj.presigned_url(:get, response_content_disposition: "attachment; filename=result_#{@result_id}.csv", expires_in: 3600)
else
nil
end
return nil unless obj.exists?

obj.presigned_url(
:get,
response_content_disposition: "attachment; filename=result_#{@result_id}.csv",
expires_in: 3600
)
end
end

def store!
Pester.s3.retry do
Retriable.with_context(:s3) do
obj = AwsS3.resource.bucket(S3_BUCKET).object(key)
obj.upload_file(filepath)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/github.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def self.send_request(verb, url, params={})
request.content_type = 'application/json'
end

http.request(request)
Retriable.retriable { http.request(request) }
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a retriable without context? what is the default behavior?

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/kamui/retriable#defaults

end

def self.get(url, params={})
Expand Down
8 changes: 2 additions & 6 deletions lib/schemas/descriptor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,8 @@ def schema_refresher

def refresh_schema
Rails.logger.info('Schemas::Descriptor.refresh_schema')
result = nil
Pester.schema_refresh.retry do
result = exec_schema_query
end

if result
if (result = Retriable.with_context(:schema_refresh) { exec_schema_query })
redis_store!(filter_tables(result.to_a))
@cache = redis_retrieve
end
Expand All @@ -84,7 +80,7 @@ def exec_schema_query

def filter_tables(schemas)
return schemas unless TABLE_BLACKLIST

schemas.reject do |column|
schema_blacklist = TABLE_BLACKLIST[column['table_schema']]
next unless schema_blacklist
Expand Down