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

Feature: let lock.extend ignore the remaining ttl. #1303

Closed

Conversation

@laixintao
Copy link
Contributor

laixintao commented Mar 10, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Lock.extend now has a new option: keep_remaining, default to False, which is identical with the old Lock.extend; When set to True, extend will set lock's ttl exactly the additional time.

A common use case would be, let's see we have 4 process and just want one to keep running, others standby. When running process is broken or offline, one of the other 3 can takeover. In order to to that, we use a lock to make sure only one prcess is running, and this process extends lock every 8 seconds before a timeout(10 seconds).

If without the new option keep_remaining(not sure if it's a good arg name that makes itself clear), the tll will get longer and longer, failover time will be unacceptable long.

Currently there are many workarounds (see #629 for example), but I think it would be better for redis-py to support this.

close #629

local expiration = redis.call('pttl', KEYS[1])
if not expiration then
expiration = 0
end

This comment has been minimized.

Copy link
@laixintao

laixintao Mar 10, 2020

Author Contributor

I checked the document: https://redis.io/commands/pttl

I am not sure when would pttl key be nil or false? Is this check necessary?

@laixintao laixintao force-pushed the laixintao:feature/with-remaining-option branch from 040ff89 to 8e9499a Mar 10, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 10, 2020

Codecov Report

Merging #1303 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1303      +/-   ##
==========================================
+ Coverage   92.81%   92.82%   +0.01%     
==========================================
  Files          19       19              
  Lines        6526     6536      +10     
==========================================
+ Hits         6057     6067      +10     
  Misses        469      469              
Impacted Files Coverage Δ
redis/lock.py 100.00% <100.00%> (ø)
tests/test_lock.py 99.45% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2f470e...9484e99. Read the comment docs.

@laixintao

This comment has been minimized.

Copy link
Contributor Author

laixintao commented Mar 10, 2020

@andymccurdy What do you think?

@laixintao laixintao changed the title Feature/with remaining option Feature: let lock.extend ignore the remaining ttl. Mar 10, 2020
Copy link
Owner

andymccurdy left a comment

Let's go with add_to_existing_ttl=True as the arg name and value to Lock.extend().

It seems far easier/less code to simply pass the add_to_existing_ttl flag into the existing LUA_EXTEND_SCRIPT as ARGS[3]. I don't recall off the top of my head if you can pass a boolean verbatim or if you have to convert it to a 1 or 0. The script can then do something like:

...
local new_ttl = ARGS[2]
if ARGS[3]    
    new_ttl += expiration
end
redis.call('pexpire', KEYS[1], new_ttl)
return 1
@laixintao

This comment has been minimized.

Copy link
Contributor Author

laixintao commented Mar 13, 2020

Sounds good, I will make the change. Don't worry I will test the lua script :)

laixintao added 6 commits Mar 10, 2020
@laixintao laixintao force-pushed the laixintao:feature/with-remaining-option branch 2 times, most recently from f3a4673 to e7a43de Mar 13, 2020
@laixintao laixintao force-pushed the laixintao:feature/with-remaining-option branch from e7a43de to 9484e99 Mar 13, 2020
@laixintao laixintao requested a review from andymccurdy Mar 13, 2020
@laixintao

This comment has been minimized.

Copy link
Contributor Author

laixintao commented Mar 24, 2020

Please take a look on this, when you got time :) @andymccurdy

@andymccurdy

This comment has been minimized.

Copy link
Owner

andymccurdy commented Mar 24, 2020

Thanks. I changed the arg name (again) and merged this in d127929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.