set a TTL on retry keys

[orenmazor]

I grabbed @dylanahsmith's changes, rebased them, and added one more change I'm evaluating right now. This adds an expiry on resque-retry keys to make sure they always get cleaned up, rather than sticking around forever.

thoughts?

[dylanahsmith]

I don't see why you lumped in my changes to this upstream pull request. I'll remove my branch though

[orenmazor]

easier for us to include both changes on our end until they both get merged here

[jzaleski]

@dylanahsmith @orenmazor lets track these changes independently for now. I am planning to merge #61 shortly.

Can you tell me more about this pull request? Is it possible that a retry could be far enough in the future that the key would expire before it's retried?

[dylanahsmith]

Is it possible that a retry could be far enough in the future that the key would expire before it's retried?

The TTL is set to @retry_delay.to_i + 60*60 so should be an hour after the the retry no matter what the delay.

[jzaleski]

Good point.

Rather than make this the default behavior (and a hard-coded 1hr), what do you think about making it configurable by setting @expire_retry_keys_after (use your judgement on whether or not this should have an attr_{reader,accessor}).

Depending on your choice of implementation it may make sense to add some validations around the value, during initialize, and raise an error if misconfigured (this has been the convention for other configuration tweaks).

I also think it may make sense to have a helper method to determine if the functionality is enabled (again depending on your implementation the could just wrap an instance variable you set during initialize):

private

def expire_retry_keys?
  !!(@expire_retry_keys_after)
end

Then your change would be something more like:

Resque.redis.expire(retry_key, @retry_delay.to_i + @expire_retry_keys_after) if expire_retry_keys?

What do you think? I look forward to seeing which way you go in an updated pull-request.

[orenmazor]

@jzaleski I hear ya on configuration. added the expire_retry_keys_after flag. not sure about disabling it completely though.

[jzaleski]

@orenmazor thank you for the updates.

Let me give you my rational behind disabling it by default..

While the expiration of keys does not necessarily seem like a breaking-change, it could be. Rather than allowing a user to opt-in to this change in behavior, we are thrusting it upon them. In general it is best to hide features behind flags between minor revisions in order to preserve backwards compatibility.

Make sense?

Beyond that, let's skip the expire_retry_keys? and attr_{reader,accessor} for the time being and focus on getting this out the door (we can always refactor this later on).

I would propose that you remove the default, and make the following update to the expiry line:

Resque.redis.expire(retry_key, @retry_delay.to_i + @expire_retry_keys_after) if @expire_retry_keys_after

[orenmazor]

@jzaleski that is a totally legitimate concern. I'm more than happy to simplify this even more :)

[orenmazor]

also a legitimate concern :) done!

[jzaleski]

Looks good. Thank you for all the hard work! Merging now