Limiting number of retries & always unlocking #7
Conversation
| @@ -22,9 +22,12 @@ def release_lock | |||
| store.delete_lock(name) | |||
| end | |||
|
|
|||
| def wait_until_lock_obtained(sleep_seconds: 3) | |||
| def wait_until_lock_obtained(sleep_seconds: 3, retries: 3) | |||
There was a problem hiding this comment.
I do agree with the fact that using a spinlock is generally not great but some people may rely on it.
As you said this would be a breaking change and I'd be better to make this change less disruptive.
just out of curiosity, how did you pick that number 3?
I'm thinking that it could be set to 20, with the current 3s sleep it'd wait for ~1min making it a bit less disruptive change.
Otherwise, setting the retries to nil and skip this new logic if retries is not set, could also be a nicer way to introduce this.
would be great to know the reasoning and use cases behind this change
There was a problem hiding this comment.
Three is just the standard for the codebase we are working in. In a high concurrency env we want to just move on to the next work unit if we are unable to obtain a lock.
Perhaps a better way to do this is to add a new method for obtain_lock and keep wait_until_lock_obtained as a spin lock.
There was a problem hiding this comment.
the base method that creates the lock is called obtain_lock
Line 13 in d0a87d4
what about creating a new method called obtain_lock_with_retries?
| @@ -110,7 +110,8 @@ def delete_lock(name) | |||
| Marloss.logger.info("Lock for #{name} deleted successfully") | |||
| end | |||
|
|
|||
| private def process_id | |||
| private | |||
There was a problem hiding this comment.
why? I personally prefer the private beside the def making it easier to read git diffs.
There was a problem hiding this comment.
In my experience calling private with arguments is uncommon. I'm not sure I have ever seen it done that way actually, this is just me reviewing the code as I went through it.
There was a problem hiding this comment.
https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/AccessModifierDeclarations I do prefer inline over group. I moved to the inline a while ago, a prefer it as is more explicit and it helps with diffs. We use it at work as well.
I actually should add rubocop sometimes this week
| @@ -58,6 +58,7 @@ def with_marloss_locker(name, opts = {}) | |||
|
|
|||
| yield(locker) | |||
|
|
|||
| ensure | |||
| raise(MissingParameterError, "DynamoDB Table not set") | ||
| elsif opts[:hash_key].nil? |
A couple of fixes: