[AOPS-66] Add a Helper method and remove member from Set on raised error

[bhcastle]

ℹ️ Most of the changes in here are due to slight refactoring. It might be easier to look commit-by-commit. The big refactor one is the very last.

Jira Ticket

https://kajabi.atlassian.net/browse/AOPS-66

Description

Nothing major is wrong here, but it would be nice to add a couple of conveniences...

Solution

• add a new helper method RedisDedupe::Set#max_member to return the largest member existing in the set, so that in the case of unique integer identifiers, that can be used to limit an ActiveRecord query.
• remove the member from the set (the member just added to the set) if the block yielded to by RedisDedupe::Set#check raises an error
• updated the gem; added and fixed up for rubocop

Customer Impact

n/a

QA Testing Guidelines

Can fire up a console and run some code there... see the specs for examples.
This won't be used until the version is specified in the main kajabi app.

Checklist

• Did you add or update specs as appropriate?
  • yes
• Are the changes behind a feature flag? (If applicable)
  • n/a
• Is there any documentation needed? (Tooltips, Help Article links, etc.)
  • no
• Did you add or edit a controller endpoint?
  • no
  • If so, are you using scoped finders & authorizing with pundit appropriately?
    • n/a

[Wheezy123]

other than @bemurphy's suggestion, LGTM, nice work! 👍

[Wheezy123]

LGTM, great work! 👍

[bemurphy]

This is one thing I was wondering about myself. Since the behavior is to re-run previously failed blocks, which could cause the loop to switch from "finishable" to "unfinishable" if the execution stoppage is repeatable, is it a major version bump perhaps? Seems like a potentially breaking change, but also, the behavior now does seem like a bug as well. Thoughts?

[cforcey]

Yeh, I am no a semantic versioning expert but think this is a good question. The interface is the same which feels like a minor version change. The modification of the exception handling feels borderline major with the new potential for duplicate execution of some block segments depending on when an exception occurs in a block with side effects. You could separate the clean refactor as a new minor version and then bump us to 0.1.0 or something for the one change to resquing if you wanted to make it more clear.

[bemurphy]

So this is the change I'm still wrapping my head around. I think the behavior looks right compared to what we have (in fact, what we have I'd say is probably a bug).

I think the biggest trail my brain is going down is, could this have a negative impact on any existing app flows? I'm thinking for instance, if we run a large execution of a collection of records having to do with mailing, and there's a failure partway through the batch that is repeatable, that execution job would never be able to finish the collection. Currently, that becomes a "skip" and the collection can finish (this has its own problems, what if we are skipping something really important). As a side note, another mechanism that comes to mind is some type of dead-letter queue.

I'm thinking one thing worth it would be to run the change by some teams with big collection/batch processes that rely on dedupe. Manage and Messaging teams come to mind. I'm interested in getting their thoughts on the behavioral change as it could impact their domains. /cc @cjbuchmann @thunderd0m3 @aburka @cforcey

[cforcey]

This is a very interesting proposed change. I agree that what we have is essentially a try_once_and_skip_on_exception sort of functionality from this class which might come as a surprise to some consumers of the gem. Your version feels closer to execute_successfully_once functionality that allows for transient errors sending an email to be recovered from by clearing the key from the set.

I am guessing your nervousness -- and mine -- comes from the wide-open nature of the block being wrapped by this helper. If it is a pipeline has several steps each with side effects, then an exception on a later step might trigger a job retry duplicating side effects up to that point. This feels a little far-fetched if what is being wrapped is a single invocation of a mailer. but some of our usages look a bit more complicated.

The API for this class is already a little odd -- RedisDedupe::Set#check does not roll off the tongue as a well-understood guarantee like run_only_once or run_successfully_only_once might. It might be total overkill, but you could offer an additional method in the current naming scheme like check_finished that wraps the check method in your rescue block.

That would allow consumers of this library to decide whether it is more important to never run this trice -- exceptionally or successfully -- or to tolerate a few retries because it is important that at least one (but perhaps more) succeed. The downside of that, of course, is that the new better behavior does not magically get applied in most of the cases where you would want it to. You could also make your method check and have it wrap check_if_duplicate_or_try_once or some such method. If someone really cared about the old behavior they could use that method (horribly named).

My last thought is that one RedisDedup is unlikely to handle all the different use cases for run_once functionality -- marketing messages already use email_deliveries to try to sort out what was sent in a first attempt and what should be retried. Our rewrites of the transactional email system will most likely involve a more nuanced record like email_delivery for those messages that track the state of a delivery attempt and use that state to prevent the dreaded multiple emails problem. Your PR inspired this small problem statement along those lines -- feedback is welcome! https://kajabi.atlassian.net/wiki/spaces/MAR/pages/1234141598/Duplicate+Email+Prevention+in+Transactional+Email

[cforcey]

I am not an expert in all the places this is used in our application, but I really appreciate the deep dive into what the current behavior amounts to when exceptions happen and the proposals for how to move forward. I shared some thoughts and would approve either this PR as is or an expansion of the API of this class that makes the new behavior the default and provides an un-rescued version of the original method for blocks with side effects that must not be repeated. Thanks for the shout out to all of us consuming this; much appreciated!

[cforcey]

#curious/nonblocking -- I like the idea of decomposing the RedisDedupe module into a Set and a Helpers file. Did you consider also leaving this attr_accessor for the client in an otherwise empty module RedisDedupe file? I found myself unsure of where client would be found and thought it might be good below the require statements in that file. Totally non-blocking!

[cforcey]

Yeh, I am no a semantic versioning expert but think this is a good question. The interface is the same which feels like a minor version change. The modification of the exception handling feels borderline major with the new potential for duplicate execution of some block segments depending on when an exception occurs in a block with side effects. You could separate the clean refactor as a new minor version and then bump us to 0.1.0 or something for the one change to resquing if you wanted to make it more clear.

[cforcey]

#nonblocking -- I like the shift of this to a guard clause since it clarifies the purpose of the check method: execute the block unless the block is a duplicate. I always get a little nervous when a ? method has side effects (setting the key and updating its expiration). Still, I think setting a key and checking the result is a well-established way to use Redis to answer global application state questions like this.

I wonder if a positive version that also trivially removes the unnecessary nil would ready better? Private methods that isolate the add and expire logic from the boolean might also clarify things.

Suggested change

	return nil unless execute_block_for_member?(member)
	return if duplicate_request?(member)

And the supporting methods might look like this:

# @param member [String, Integer]  member identifying value to make sure the
#   given block only runs once
# @return [Boolean] when a member cannot be added to the set, it is considered a duplicate
def duplicate_request?(member)
  (added_to_set, _expiration_updated) = add_key_and_update_ttl(member)
  !added_to_set
end

# @param member [String, Integer]  member identifying value to make sure the
#   given block only runs once
# @return [Array<Boolean>] a list of pipeline results, the first element of which
#   will be true or false depending on whether or not the add operation was
#   successful. If false, the requested member should be considered a duplicate.
def add_key_and_update_ttl(member)
  redis.pipelined do
    redis.sadd(key, member)
    redis.expire(key, expires_in)
  end
end

Totally optional and maybe not even an improvement. I personally needed to think pretty hard about why "unless execute_block_for_member?(member)" should cause the block not to run, as well as look up why results[0] was a boolean indicating the sadd succeeded or failed. Might just be my rusty Redis brain.

[cforcey]

This is a very interesting proposed change. I agree that what we have is essentially a try_once_and_skip_on_exception sort of functionality from this class which might come as a surprise to some consumers of the gem. Your version feels closer to execute_successfully_once functionality that allows for transient errors sending an email to be recovered from by clearing the key from the set.

I am guessing your nervousness -- and mine -- comes from the wide-open nature of the block being wrapped by this helper. If it is a pipeline has several steps each with side effects, then an exception on a later step might trigger a job retry duplicating side effects up to that point. This feels a little far-fetched if what is being wrapped is a single invocation of a mailer. but some of our usages look a bit more complicated.

The API for this class is already a little odd -- RedisDedupe::Set#check does not roll off the tongue as a well-understood guarantee like run_only_once or run_successfully_only_once might. It might be total overkill, but you could offer an additional method in the current naming scheme like check_finished that wraps the check method in your rescue block.

That would allow consumers of this library to decide whether it is more important to never run this trice -- exceptionally or successfully -- or to tolerate a few retries because it is important that at least one (but perhaps more) succeed. The downside of that, of course, is that the new better behavior does not magically get applied in most of the cases where you would want it to. You could also make your method check and have it wrap check_if_duplicate_or_try_once or some such method. If someone really cared about the old behavior they could use that method (horribly named).

My last thought is that one RedisDedup is unlikely to handle all the different use cases for run_once functionality -- marketing messages already use email_deliveries to try to sort out what was sent in a first attempt and what should be retried. Our rewrites of the transactional email system will most likely involve a more nuanced record like email_delivery for those messages that track the state of a delivery attempt and use that state to prevent the dreaded multiple emails problem. Your PR inspired this small problem statement along those lines -- feedback is welcome! https://kajabi.atlassian.net/wiki/spaces/MAR/pages/1234141598/Duplicate+Email+Prevention+in+Transactional+Email