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

Add a simple context plugin #43

Merged
merged 6 commits into from
Jul 28, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 4 additions & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Style/Documentation:

Style/TrailingCommaInArguments:
EnforcedStyleForMultiline: comma

Style/TrailingCommaInLiteral:
EnforcedStyleForMultiline: comma

Expand All @@ -19,6 +19,9 @@ Style/IndentArray:
Style/IndentHash:
Enabled: false

Style/NegatedIf:
Enabled: false

Metrics/ClassLength:
Enabled: false

Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## HEAD

* Added contexts feature. Thanks to @apurvis.
Copy link
Contributor

Choose a reason for hiding this comment

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

you're missing a version number here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh what I usually do is put new feature/bug fixes in HEAD, and then when I cut a new version, I'll move the new stuff into that version. Since sometimes I collect a few features or bug fixes before doing a version.

Copy link
Contributor

Choose a reason for hiding this comment

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

whatever you prefer; i'm just used to always having the next version being the first line of the changelog

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't always know if the next version will be major, minor, or patch until I know everything going into it. So I keep it HEAD until I cut the version.


## 3.0.2

* Add configuration and options validation.
Expand Down
47 changes: 47 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,49 @@ ensure
end
```

## Contexts

Contexts allow you to coordinate sets of Retriable options across an application. Each context is basically an argument hash for `Retriable.retriable` that is stored in the `Retriable.config` as a simple `Hash` and is accessible by name. For example:

```ruby
Retriable.configure do |c|
c.context[:aws] = {
tries: 3,
base_interval: 5,
on_retry: Proc.new { puts 'Curse you, AWS!' }
}
c.context[:mysql] = {
tries: 10,
multiplier: 2.5,
on: Mysql::DeadlockException
}
end
```

This will create two contexts, `aws` and `mysql`, which allow you to reuse different backoff strategies across your application without continually passing those strategy options to the `retriable` method.

These are used simply by calling `Retriable.retriable_with_context`:

```ruby
# Will retry all exceptions
Retriable.retriable_with_context(:aws) do
# aws_call
end

# Will retry Mysql::DeadlockException
Retriable.retriable_with_context(:mysql) do
# write_to_table
end
```

You can even temporarily override individual options for a configured context:

```ruby
Retriable.retriable_with_context(:mysql, tries: 30) do
# write_to_table
end
```

## Kernel Extension

If you want to call `Retriable.retriable` without the `Retriable` module prefix and you don't mind extending `Kernel`,
Expand All @@ -246,6 +289,10 @@ and then you can call `#retriable` in any context like this:
retriable do
# code here...
end

retriable_with_context(:api) do
# code here...
end
```

## Proxy Wrapper Object
Expand Down
10 changes: 9 additions & 1 deletion lib/retriable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,22 @@
module Retriable
module_function

def self.configure
def configure
yield(config)
end

def config
@config ||= Config.new
end

def retriable_with_context(context_key, options = {}, &block)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could even just name the method with_context? then you have Retriable.with_context... reads pretty well, actually.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I named it with_context initially, but the problem is I didn't like that if you use the kernel extension that you couldn't really put with_context on the Kernel since it's too generic. I didn't like the idea of having it be #retriable map to Retriable.retriable but #retriable_with_context in Kernel map to Retriable.with_context.

I agree retriable_with_context is kind of verbose, I'm still thinking about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't like the idea of having it be #retriable map to Retriable.retriable but #retriable_with_context in Kernel map to Retriable.with_context.

i tend to view adding methods to Kernel as kind of way more intrusive than, for instance, implementing method_missing within the gem... i would probably vote to deprecate it altogether if i had my druthers (which i don't; just putting that out there)

point being, i think Retriable.with_context reads really well, and if the kernel extension can't match the semantics, it doesn't really bother me (though i agree it's not quite as neat & tidy)

Copy link
Contributor

Choose a reason for hiding this comment

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

retriable is honestly a pretty generic thing to be injecting into Kernel - no more or less generic, at least, than with_context IMHO

Copy link
Owner Author

@kamui kamui Jul 28, 2017

Choose a reason for hiding this comment

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

If I had to write it again I might leave out the kernel extension, but I don't want to break it for people upgrading. I changed it to name it with_context unless you use the kernel extension, then it's retriable_with_context.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah you'd have to deprecate it and then wait until the next major version... even then it might not be worth breaking people's shit.

if you were considering that approach though, you could deprecate it now.

if !config.context.key?(context_key)
raise ArgumentError, "#{context_key} not found in Retriable.config.context"
end

retriable(config.context[context_key].merge(options), &block) if block
end

def retriable(opts = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're going to declare the &block in the with_retriable args, might as well add it here too. might help generations to come read the code slightly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think i need the block here because we call yield here. retriable_with_context needs it because I need to pass the block into #retriable without calling it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah you don't need it, but it does make it clear that the method takes a block - just makes the method signature fully explicit because otherwise the yield is kinda buried way in there (where it needs to be, but it's a little hard to spot at first). up to you really, though.

local_config = opts.empty? ? config : Config.new(config.to_h.merge(opts))

Expand Down
2 changes: 2 additions & 0 deletions lib/retriable/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class Config
:timeout,
:on,
:on_retry,
:context,
Copy link
Contributor

Choose a reason for hiding this comment

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

plural contexts, since there are multiple?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I'll change that.

].freeze

attr_accessor(*ATTRIBUTES)
Expand All @@ -27,6 +28,7 @@ def initialize(opts = {})
@timeout = nil
@on = [StandardError]
@on_retry = nil
@context = {}

opts.each do |k, v|
raise ArgumentError, "#{k} is not a valid option" if !ATTRIBUTES.include?(k)
Expand Down
4 changes: 4 additions & 0 deletions lib/retriable/core_ext/kernel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@ module Kernel
def retriable(opts = {}, &block)
Retriable.retriable(opts, &block)
end

def retriable_with_context(context_key, opts = {}, &block)
Retriable.retriable_with_context(context_key, opts, &block)
end
end
4 changes: 4 additions & 0 deletions spec/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
expect(subject.new.on_retry).must_be_nil
end

it "context defaults to {}" do
expect(subject.new.context).must_equal Hash.new
end

it "raises errors on invalid configuration" do
assert_raises ArgumentError do
subject.new(does_not_exist: 123)
Expand Down
55 changes: 55 additions & 0 deletions spec/retriable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -376,4 +376,59 @@ class SecondTestError < TestError; end
Retriable.retriable(does_not_exist: 123)
end
end

describe "#retriable_with_context" do
before do
Retriable.configure do |c|
c.sleep_disabled = true
c.context[:sql] = { tries: 1 }
c.context[:api] = { tries: 3 }
end
end

it "sql context stops at first try if the block does not raise an exception" do
tries = 0
subject.retriable_with_context(:sql) do
tries += 1
end

expect(tries).must_equal 1
end

it "retriable_with_context respects the context options" do
tries = 0

expect do
subject.retriable_with_context(:api) do
tries += 1
raise StandardError.new, "StandardError occurred"
end
end.must_raise StandardError

expect(tries).must_equal 3
end

it "retriable_with_context allows override options" do
tries = 0

expect do
subject.retriable_with_context(:sql, tries: 5) do
tries += 1
raise StandardError.new, "StandardError occurred"
end
end.must_raise StandardError

expect(tries).must_equal 5
end

it "raises an ArgumentError when the context isn't found" do
tries = 0

expect do
subject.retriable_with_context(:wtf) do
tries += 1
end
end.must_raise ArgumentError
end
end
end