Add a simple context plugin #43

kamui · 2017-07-27T03:15:16Z

This was inspired by this PR by @apurvis: #39

In reviewing that PR there were a few design decisions that I wasn't a fan of, but in reviewing it I didn't have an immediate suggestion or alternative. The issues for me where:

I'm not a fan of subclassing Hash. Core classes are usually heavily or partially written in C for speed and can behave in unexpected ways. I prefer composition or delegation when that can be utilized, in this approach, I just made context a simple hash composed on the Retiable module.
I don't like using method_missing on Retriable just to support context, using a simple Hash made it pretty easy to call using the key in a with_context method.
I didn't like having to patch config with contexts, this approach separates config from context without context support having to touch the config object at all.
To me this is a lot simpler to read and understand, although you could argue the API is a more verbose.

I spiked on experimenting with a simpler implementation and came to this. Using that PR's example:

Config:

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

Usage:

# Will retry all exceptions
Retriable.with_context(:aws) do
  # aws_call
end

Retriable.with_context(:mysql, tries: 30) do
  # write_to_table
end

apurvis · 2017-07-27T05:19:07Z

i wasn't super stoked about subclassing Hash at the time either; the real upside was that it let you validate the arguments/configuration easily but it wasn't a huge upside.
matter of preference i guess; i think it's one of the only decent places i've found to use method_missing profitably, though with_context makes sense too. just more verbose.
this i have a little more issue with - contexts are definitely part of the config, no? in fact the way we want to use them, they are like the most important part of the configuration of this gem. but i guess more importantly, if i were a new user to the gem, i would expect to find configuration in... the configuration object?

what do you think?

kamui · 2017-07-27T06:07:40Z

So the way I look at it, if this is released as a plugin and you have to explicitly require another file to gain a feature, it doesn't seem so odd to have your context object be a different block arg than the config arg. I think it provides a clean separation and delineation between core functionality and plugin. If you required it and didn't touch context or call with_context, it would work exactly as is without really much performance or resource penalty. The core code paths remain exactly the same.

Now that I think about this, because the cost of just having context always required is extremely minimal (single hash allocation) and doesn't do anything really unless you call with_context, I could just put the context hash in config, and then you set it with the config block arg and it just comes as part of core, it's no longer a plugin.

I originally thought a plugin was a good path because I was worried the implementation much touch or alter the core code paths for a feature a lot of people might not use. I thought this could potentially make the core code paths more complicated. I didn't like that potential trade off. This approach just adds an optional hash allocation to config, and with_context is a helper method that doesn't add overhead/complexity to the core function.

What do you think?

apurvis · 2017-07-27T06:19:43Z

i was going to post some suggestion more or less exactly like that but i was hoping you'd come to that conclusion yourself.

apurvis

minor comments

apurvis · 2017-07-27T07:02:23Z

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## HEAD

+* Added contexts feature. Thanks to @apurvis.


you're missing a version number here?

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.

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

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.

apurvis · 2017-07-27T07:02:51Z

lib/retriable.rb

    yield(config)
  end

  def config
    @config ||= Config.new
  end

+  def retriable_with_context(context_key, options = {}, &block)


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

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.

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)

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

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.

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.

apurvis · 2017-07-27T07:03:18Z

lib/retriable/config.rb

@@ -9,6 +9,7 @@ class Config
      :timeout,
      :on,
      :on_retry,
+      :context,


plural contexts, since there are multiple?

Yeah, I'll change that.

apurvis · 2017-07-27T07:16:58Z

lib/retriable.rb

+
+    retriable(config.context[context_key].merge(options), &block) if block
+  end
+
  def retriable(opts = {})


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.

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.

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.

apurvis

looks good to me; you know my thoughts on the final naming decision but overall 👍

apurvis · 2017-07-28T05:09:54Z

so stoked this feature will finally get in there - glad you ultimately realized it's not that big a change - but i think it will see a ton of usage.

your dominance over Retryable is assured

…nel extension to keep it descriptive

kamui · 2017-07-28T05:11:44Z

Already have more stars, so that's a good sign ;D

apurvis · 2017-07-28T05:23:38Z

pretty unrelated, but i've been using "squash+merge" more than just plain old merge to keep the commit history of my projects a bit cleaner. YMMV

kamui · 2017-07-28T05:28:56Z

I use squash merge as well.

apurvis · 2017-07-28T05:29:24Z

lib/retriable.rb

    yield(config)
  end

  def config
    @config ||= Config.new
  end

+  def with_context(context_key, options = {}, &block)
+    if !config.contexts.key?(context_key)
+      raise ArgumentError, "#{context_key} not found in Retriable.config.contexts"


you could maybe make this exception more useful by listing #{Retriable.config.keys} /shrug

…t key that doesn't exist

apurvis · 2017-07-28T05:37:05Z

lib/retriable.rb

    yield(config)
  end

  def config
    @config ||= Config.new
  end

+  def with_context(context_key, options = {}, &block)
+    if !config.contexts.key?(context_key)
+      raise ArgumentError, "#{context_key} not found in Retriable.config.contexts. Here the available contexts: #{config.contexts.keys}"


"Here are the...." ;)

or just say "Available contexts: "

Damn, I'm getting sleepy and wanted to cut a new version. I'll bundle the typo fix in with the next patch version!

kamui mentioned this pull request Jul 27, 2017

Optional contexts feature #39

Closed

Add a simple context plugin

94b1def

kamui force-pushed the simple-context branch from 5702707 to 94b1def Compare July 27, 2017 06:10

Move context feature into core

4e35ddc

apurvis reviewed Jul 27, 2017

View reviewed changes

Pluralize config.context to config.contexts

43da6cd

apurvis approved these changes Jul 28, 2017

View reviewed changes

Rename retriable_with_context to with_context, unless it's in the ker…

ea5b10f

…nel extension to keep it descriptive

kamui force-pushed the simple-context branch from 6de4197 to ea5b10f Compare July 28, 2017 05:10

Remove unnecessary trailing comma in arg list

aeb16a7

apurvis reviewed Jul 28, 2017

View reviewed changes

Add available contexts in the exception message when calling a contex…

de3fcb8

…t key that doesn't exist

kamui merged commit d9976a8 into master Jul 28, 2017

kamui deleted the simple-context branch July 28, 2017 05:34

apurvis reviewed Jul 28, 2017

View reviewed changes

apurvis mentioned this pull request Mar 25, 2019

Provide a context manager jd/tenacity#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a simple context plugin #43

Add a simple context plugin #43

kamui commented Jul 27, 2017 •

edited

Loading

apurvis commented Jul 27, 2017

kamui commented Jul 27, 2017

apurvis commented Jul 27, 2017

apurvis left a comment

apurvis Jul 27, 2017

kamui Jul 28, 2017

apurvis Jul 28, 2017

kamui Jul 28, 2017

apurvis Jul 27, 2017

kamui Jul 28, 2017

apurvis Jul 28, 2017

apurvis Jul 28, 2017

kamui Jul 28, 2017 •

edited

Loading

apurvis Jul 28, 2017

apurvis Jul 27, 2017

kamui Jul 28, 2017

apurvis Jul 27, 2017

kamui Jul 28, 2017

apurvis Jul 28, 2017

apurvis left a comment

apurvis commented Jul 28, 2017

kamui commented Jul 28, 2017

apurvis commented Jul 28, 2017

kamui commented Jul 28, 2017

apurvis Jul 28, 2017

apurvis Jul 28, 2017

kamui Jul 28, 2017 •

edited

Loading

		@@ -1,5 +1,7 @@
		## HEAD

		* Added contexts feature. Thanks to @apurvis.

Add a simple context plugin #43

Add a simple context plugin #43

Conversation

kamui commented Jul 27, 2017 • edited Loading

apurvis commented Jul 27, 2017

kamui commented Jul 27, 2017

apurvis commented Jul 27, 2017

apurvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamui Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apurvis left a comment

Choose a reason for hiding this comment

apurvis commented Jul 28, 2017

kamui commented Jul 28, 2017

apurvis commented Jul 28, 2017

kamui commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamui Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

kamui commented Jul 27, 2017 •

edited

Loading

kamui Jul 28, 2017 •

edited

Loading

kamui Jul 28, 2017 •

edited

Loading