Add NX/XX/GT/LT options to EXPIRE command group #2795

sunng87 · 2015-10-03T02:39:35Z

Add NX, XX, GT, and LT flags to EXPIRE, PEXPIRE, EXPIREAT, PEXAPIREAT.

NX - only modify the TTL if no TTL is currently set
XX - only modify the TTL if there is a TTL currently set
GT - only increase the TTL (considering non-volatile keys as infinite expire time)
LT - only decrease the TTL (considering non-volatile keys as infinite expire time)
return value of the command is 0 when the operation was skipped due to one of these flags.

Just some quick work to add an NX option to expire commands. This allows expiration to be set for only once, just like setnx. In our application, we have some zsets and hashes that update frequently, but expire at a fixed time defined at its creation.

Searched issue history, I also found some others who had similar requirements: #1840

This is just the first commit. If you think it's OK I will add docs and tests for this feature. Thanks!

EDIT: spelling

ptaoussanis · 2015-10-10T07:31:04Z

+1, this would be handy

yoav-steinberg · 2021-07-15T12:23:00Z

@sunng87 like this. Can you rebase and add a test.
@oranagra @itamarhaber WDYT?

itamarhaber · 2021-07-15T12:37:06Z

Definitely useful for saving a 7-liner scriprt - I'm for it.

My only beef is the naming - NX means "if not exists" elsewhere in Redis, whereas here we're talking about "if no TTL/expiry". So, mebbe NE or NT?

oranagra · 2021-07-18T06:29:02Z

I agree this would be nice to have.
In addition to the NX feature, maybe it would also be useful to gave GT and LT (grater than / less than) feature.
My other concern is that as soon as e add these argument flags, it would mean that we can't change the command to be variadic in the future (taking multiple key names).
Also, looking at the linked issue, there was a request to return the TTL rather than 0 / 1.
@redis/core-team please share your thoughts.

sunng87 · 2021-07-18T07:44:34Z

Can you rebase and add a test.

No problem. Let me catch up

Signed-off-by: Ning Sun <[email protected]>

sunng87 · 2021-07-18T13:06:53Z

My only beef is the naming - NX means "if not exists" elsewhere in Redis, whereas here we're talking about "if no TTL/expiry". So, mebbe NE or NT?

NX here means if a TTL not exists. I just followed previous attempts of this feature for naming. And I'm pretty open for other suggestions.

In addition to the NX feature, maybe it would also be useful to gave GT and LT (grater than / less than) feature.

~~For now I don't have a use case for GT and LT but the code base is open for adding more flags here.~~ I just realized LT is to fix same issue with my NX.

My other concern is that as soon as e add these argument flags, it would mean that we can't change the command to be variadic in the future (taking multiple key names). Also, looking at the linked issue, there was a request to return the TTL rather than 0 / 1.

These two tasks can be done via pipeline as a workaround (pipelining multiple expire and ttl command). So for now I think it's not very urgent.

Signed-off-by: Ning Sun <[email protected]>

oranagra · 2021-07-18T14:15:48Z

@sunng87 you mean that LT would fix your problem the same way that NX does, but there are probably use cases that will benefit from one and not from the other.
In fact, now that i think of it, there are probably also use cases for XX (modify only if already volatile).

sunng87 · 2021-07-18T15:15:42Z

I agree and there are always possibilities. But before we have real scenario I would suggest to keep the code simple and open for new options.

yossigo · 2021-07-18T15:25:07Z

@oranagra I think that having smarter EXPIRE is more useful than having it variadic, which I believe has very little value over pipelining.

sunng87 · 2021-07-19T02:19:24Z

The patch is ready for review.

oranagra · 2021-07-19T06:41:05Z

I think we wanna take this opportunity to add all the other useful options.
and we still need to conclude if we can / want to change the response type when any of these options are used (as requested in the linked issue).
maybe that's valid to do similarly as how SPOP changes return value when COUNT is added, or maybe we can add a GETTTL argument.
let's wait for further feedback..

yoav-steinberg · 2021-07-20T06:41:43Z

LT/GT/XX sound good to me.
Regarding return value, if I'm correct 0 can still be a valid value telling us the key expired or never existed while anything larger than 0 will give us the current TTL (this will need to be adjusted to unixtime for AT variants and millisecs for P variants). This seems like a relatively small compatibility break, so I'm for it.

oranagra

thank you for the efforts.
i added a few comments. some are suggestion for improvements, others are open for discussion, let me know what you think.

src/expire.c

oranagra · 2021-07-21T05:23:29Z

src/expire.c

+    if (flag & FLAG_NX) {
+        current_expire = getExpire(c->db, key);
+        if (current_expire != -1) {
+            addReply(c,shared.czero);


the docs say that 0 is returned when the key doesn't exist.
i suppose it makes sense to at least let the caller distinguish between non-existing key and a skipped operation due to flag.
one option could have been to return -1 in these cases, but considering that this is inconsistent with the -1 and -2 values returned by the TTL command, maybe that's not a wise idea.

maybe like the linked issue asks for, we better add a way to just return the TTL.
it's true that the user can pipeline a TTL command right after this one, but i suppose it'll be more convenient to let them use this in one go.

I suggest to add a GET flag, which will change the return value from 0/1 to the same return value TTL command group has (see ttlGenericCommand). i.e. it would be relative/absolute and ms/sec depending on the EXPIRE variant that was used, and we'll have to also return -2 and -1 in the appropriate places.

p.s. another wild idea is to also support an INCR flag (for the non-absolute expire variants, when basetime is not 0), in which case we increment the existing expiration time (similarly to the ZADD feature).

I still suggest not to use the return value of EXPIRE commands for TTL. It introduces additional complexity for various clients to distinguish return code and value for this write operation, that defined by an optional flag.

And also I think returning 0 makes sense for this new scenario. We can extend its meaning along with these new features. The 0 means nothing changed with this write operation, it can be a key not exist or the operation is skipped.

ok. we can stop here. already a big step forward.
GET and INCR arguments can be added later if we wish (nothing here that prevents that)

src/expire.c

tests/unit/expire.tcl

src/expire.c

Signed-off-by: Ning Sun <[email protected]>

src/expire.c

Signed-off-by: Ning Sun <[email protected]>

oranagra

LGTM.. few minor suggestions for improvement.

src/expire.c

tests/unit/expire.tcl

sunng87 · 2021-07-25T13:35:36Z

@oranagra All issues resolved. Thanks for your patient and great suggestions.

src/expire.c

oranagra · 2021-07-25T14:30:45Z

@sunng87 thanks..
can you also make a PR for https://github.com/redis/redis-doc/pulls ?

oranagra · 2021-07-26T06:58:13Z

@sunng87 please merge my indentation fixes suggestions (it seems this PR doesn't let me edit the code)
@redis/core-team please approve the new flags for the EXPIRE group commands.

yossigo

LGTM, added a few minor typo/formatting comments.

src/expire.c

tests/unit/expire.tcl

Signed-off-by: Ning Sun <[email protected]>

sunng87 · 2021-07-26T16:08:14Z

Doc changes created at redis/redis-doc#1613

madolson

API and code LGTM.

Add NX, XX, GT, and LT flags to EXPIRE, PEXPIRE, EXPIREAT, PEXAPIREAT. - NX - only modify the TTL if no TTL is currently set - XX - only modify the TTL if there is a TTL currently set - GT - only increase the TTL (considering non-volatile keys as infinite expire time) - LT - only decrease the TTL (considering non-volatile keys as infinite expire time) return value of the command is 0 when the operation was skipped due to one of these flags. Signed-off-by: Ning Sun <[email protected]>

yoav-steinberg added the state:needs-test-code the PR is missing test code label Jul 15, 2021

sunng87 force-pushed the expire-nx branch from 9bfa7f4 to c5ce083 Compare July 18, 2021 08:13

add nx option to expire commands

5d71457

Signed-off-by: Ning Sun <[email protected]>

sunng87 force-pushed the expire-nx branch from c5ce083 to 5d71457 Compare July 18, 2021 08:18

sunng87 added 2 commits July 18, 2021 16:35

add test and comments for expire NX option

f51e4b6

refactor command option parser for expire

47765e5

Signed-off-by: Ning Sun <[email protected]>

improve comments

88ea4da

Signed-off-by: Ning Sun <[email protected]>

oranagra reviewed Jul 21, 2021

View reviewed changes

sundb reviewed Jul 21, 2021

View reviewed changes

src/expire.c Outdated Show resolved Hide resolved

yoav-steinberg reviewed Jul 21, 2021

View reviewed changes

src/expire.c Outdated Show resolved Hide resolved

add support for XX/GT/LT

63e56d6

Signed-off-by: Ning Sun <[email protected]>

sunng87 force-pushed the expire-nx branch from e17a42c to 63e56d6 Compare July 21, 2021 15:58

oranagra reviewed Jul 22, 2021

View reviewed changes

src/expire.c Outdated Show resolved Hide resolved

addressing review issues

fa6f221

oranagra reviewed Jul 22, 2021

View reviewed changes

src/expire.c Outdated Show resolved Hide resolved

src/expire.c Outdated Show resolved Hide resolved

src/expire.c Outdated Show resolved Hide resolved

addressing review issues and fix bad test cases

146b25b

Signed-off-by: Ning Sun <[email protected]>

sunng87 force-pushed the expire-nx branch from 50a4aa3 to 146b25b Compare July 23, 2021 14:28

oranagra reviewed Jul 25, 2021

View reviewed changes

src/expire.c Show resolved Hide resolved

src/expire.c Outdated Show resolved Hide resolved

oranagra linked an issue Jul 26, 2021 that may be closed by this pull request

Add 'NX' option to 'EXPIRE' command set #1840

Closed

oranagra changed the title ~~(feat) added nx option to expire commands~~ Add NX/XX/GT/LT options to EXPIRE command group Jul 26, 2021

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository labels Jul 26, 2021

yossigo previously approved these changes Jul 26, 2021

View reviewed changes

sunng87 dismissed yossigo’s stale review via 4bca6c0 July 26, 2021 12:56

sunng87 force-pushed the expire-nx branch 2 times, most recently from 4bca6c0 to ff3d321 Compare July 26, 2021 13:00

addressing remaining review comments for expiry options feature

7baf24a

Signed-off-by: Ning Sun <[email protected]>

sunng87 force-pushed the expire-nx branch from ff3d321 to 7baf24a Compare July 26, 2021 13:11

oranagra approved these changes Jul 26, 2021

View reviewed changes

sunng87 mentioned this pull request Jul 26, 2021

Add doc for new options of expire commands redis/redis-doc#1613

Merged

madolson approved these changes Aug 1, 2021

View reviewed changes

oranagra merged commit f74af0e into redis:unstable Aug 2, 2021

mp911de mentioned this pull request Oct 29, 2021

Add NX/XX/GT/LT options to EXPIRE commands redis/lettuce#1905

Closed

viralpraxis mentioned this pull request Nov 3, 2023

Add support for EXPIRE command options sds/mock_redis#286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NX/XX/GT/LT options to EXPIRE command group #2795

Add NX/XX/GT/LT options to EXPIRE command group #2795

sunng87 commented Oct 3, 2015 •

edited by oranagra

Loading

ptaoussanis commented Oct 10, 2015

yoav-steinberg commented Jul 15, 2021

itamarhaber commented Jul 15, 2021

oranagra commented Jul 18, 2021

sunng87 commented Jul 18, 2021

sunng87 commented Jul 18, 2021 •

edited

Loading

oranagra commented Jul 18, 2021

sunng87 commented Jul 18, 2021

yossigo commented Jul 18, 2021

sunng87 commented Jul 19, 2021

oranagra commented Jul 19, 2021

yoav-steinberg commented Jul 20, 2021

oranagra left a comment

oranagra Jul 21, 2021

sunng87 Jul 24, 2021

oranagra Jul 25, 2021

oranagra left a comment

sunng87 commented Jul 25, 2021

oranagra commented Jul 25, 2021

oranagra commented Jul 26, 2021

yossigo left a comment

sunng87 commented Jul 26, 2021

madolson left a comment

Add NX/XX/GT/LT options to EXPIRE command group #2795

Add NX/XX/GT/LT options to EXPIRE command group #2795

Conversation

sunng87 commented Oct 3, 2015 • edited by oranagra Loading

ptaoussanis commented Oct 10, 2015

yoav-steinberg commented Jul 15, 2021

itamarhaber commented Jul 15, 2021

oranagra commented Jul 18, 2021

sunng87 commented Jul 18, 2021

sunng87 commented Jul 18, 2021 • edited Loading

oranagra commented Jul 18, 2021

sunng87 commented Jul 18, 2021

yossigo commented Jul 18, 2021

sunng87 commented Jul 19, 2021

oranagra commented Jul 19, 2021

yoav-steinberg commented Jul 20, 2021

oranagra left a comment

Choose a reason for hiding this comment

oranagra Jul 21, 2021

Choose a reason for hiding this comment

sunng87 Jul 24, 2021

Choose a reason for hiding this comment

oranagra Jul 25, 2021

Choose a reason for hiding this comment

oranagra left a comment

Choose a reason for hiding this comment

sunng87 commented Jul 25, 2021

oranagra commented Jul 25, 2021

oranagra commented Jul 26, 2021

yossigo left a comment

Choose a reason for hiding this comment

sunng87 commented Jul 26, 2021

madolson left a comment

Choose a reason for hiding this comment

sunng87 commented Oct 3, 2015 •

edited by oranagra

Loading

sunng87 commented Jul 18, 2021 •

edited

Loading