GETEX, GETDEL and SET PXAT/EXAT #8327

nmvk · 2021-01-13T21:57:30Z

This commit introduces two new command and two options for an existing command

GETEX <key> [PERSIST][EX seconds][PX milliseconds] [EXAT seconds-timestamp][PXAT milliseconds-timestamp]

The getexCommand() function implements extended options and variants of the GET command. Unlike GET command this command is not read-only. Only one of the options can be used at a given time.

PERSIST removes any TTL associated with the key.
EX Set expiry TTL in seconds.
PX Set expiry TTL in milliseconds.
EXAT Same like EX instead of specifying the number of seconds representing the TTL (time to live), it takes an absolute Unix timestamp
PXAT Same like PX instead of specifying the number of milliseconds representing the TTL (time to live), it takes an absolute Unix timestamp

Command would return either the bulk string, error or nil.

GETDEL <key>

Would delete the key after getting.

SET key value [NX] [XX] [KEEPTTL] [GET] [EX <seconds>] [PX <milliseconds>] [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>]

Two new options added here are EXAT and PXAT

Key implementation notes

SET with PX/EX/EXAT/PXAT is always translated to PXAT in AOF. When relative time is specified (PX/EX), replication will always use PX.
setexCommand and psetexCommand would no longer need translation in feedAppendOnlyFile as they are modified to invoke setGenericCommand with appropriate flags which will take care of correct AOF translation.
GETEX without any optional argument behaves like GET.
GETEX command is never propagated, It is either propagated as PEXPIRE[AT], or PERSIST.
GETDEL command is propagated as DEL
Combined the validation for SET and GETEX arguments.
Test cases to validate AOF/Replication propagation

Issue - #2762

src/t_string.c

madolson · 2021-01-18T17:06:16Z

@redis/core-team

Hey, this should be ready for review. I looked through the command code, but would appreciate another pair of eyes at least since that is not the code I'm more comfortable with.

oranagra

@nmvk thanks for this PR.
added a few comments that should be trivial to solve.

the only one that would probably require some work is about not propagating absolute expire times to replicas.

src/server.c

src/t_string.c

tests/unit/type/string.tcl

soloestoy · 2021-01-19T09:34:24Z

IMHO MULTI/EXEC and lua script can do it better, but if users really wanna it, I prefer some clear and specific commands like GETEX/PGETEX/GETDEL as mentioned in the original issue, it's not easy to understand and use GETEX with so much flags, especially these flags have many conflict.

oranagra · 2021-01-19T12:08:24Z

@soloestoy indeed it can be achieved by MULTI exec, but this will make it more convenient for users, and AFAIK it was requested a lot.

The problem with splitting it to many commands is that whenever we wanna add a new option, we need to add it to all of them, or add another variant of the command.
Redis did abandon SETEX, SETNX, SETXX a long time ago in favor of SET with various arguments.
and we recently abandoned GETSET (to avoid adding an EX argument to it) in favor of SET with GET argument, and did the same to many many other commands (ZRANGE, GEORADIUS*, PUPPUSH).

so as long as the GETEX command is always a write command and always refers to one key, and always returns the value, i think we should stick with one command rather than several.

oranagra

i think we must add specific tests for the replication and AOF propagation of all the GETEX variants.
including:

making sure GETEX with no arguments isn't propagated
use of PERSIST
EX/PX/EXAT/PXAT
DEL

src/aof.c

src/t_string.c

src/aof.c

…#8357) This commit adds tests to make sure that relative and absolute expire commands are propagated as is to replicas and stop any future attempt to change that without a proper discussion. see #8327 and #5171 Additionally it slightly improve the AOF test that tests the opposite (always propagating absolute times), by covering more commands, and shaving 2 seconds from the test time.

This commit introduces one new command and two options for an existing command GETEX <key> [KEEPTTL][PERSIST][DEL][EX seconds][PX milliseconds] [EXAT seconds-timestamp][PXAT milliseconds-timestamp] The getexCommand() function implements extended options and variants of the GET command. Unlike GET command this command is not read-only. Only one of the options can be used at a given time. 1. KEEPTTL is the default behavior and does not alter any TTL. 2. PERSIST removes any TTL associated with the key. 3. DEL deletes the key after the return. 4. EX Set expiry TTL in seconds. 5. PX Set expiry TTL in milliseconds. 6. EXAT Same like EX instead of specifying the number of seconds representing the TTL (time to live), it takes an absolute Unix timestamp 7. PXAT Same like PX instead of specifying the number of milliseconds representing the TTL (time to live), it takes an absolute Unix timestamp Command would either return the bulk string, error or nil. SET key value [NX] [XX] [KEEPTTL] [GET] [EX <seconds>] [PX <milliseconds>] [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] The two new options added here are EXAT and PXAT

Rewrote the propagation to use relative TTL when PX/EX is specified. AOF will transalte the PX to SET with PEXPIREAT. If PXAT/EXAT time has elapsed in GETEX, performining delete now. Changed default behavior of GETEX to be same as GET. Fixed other PR comments.

Replication and AOF test coverage for different options of GETEX. Modified the existing expiration test to include SET PXAT/EXAT and GETEX. Added a test to ensure if GETEX is invoked with no arguments then not to write anything to AOF file.

oranagra · 2021-01-20T12:09:50Z

@nmvk please avoid pushing a rebase and an actual change (editing old commits) in the same force-push, it's very hard for me to find your changes and review them.

oranagra

minor suggestions for improvements and then i think this is ready to be merged.

src/aof.c

src/t_string.c

tests/unit/expire.tcl

Updated comments from PR review. Updated GETEX PERSIST propagate to replica test to include DEL case.

yossigo · 2021-01-25T22:43:30Z

Okay the long version then :)

GETX seems confusing, creates yet another command notation which I don't think we want at this point.
POP implies an operation on a collection, I think it's confusing.
TAKE is a bit awkward for me as it doesn't seem related in any way to GET.
GETEX is not perfect, it's a bit ambiguous as it is more easy to interpret as "expire" rather than "extended", but I can't come up with something better. At some point I did toy with GETMUT (mutable get) but:
- It would make more sense if we also give up GETDEL and use a DEL argument, which is not good introspection-wise
- Is also awkward
- Steers this discussion further from a conclusion

src/t_string.c

tests/unit/type/string.tcl

Update the PR.

src/t_string.c

Propagate as UNLINK

oranagra · 2021-01-26T05:39:48Z

Ok, next. Who wants to make a redis-doc PR?

nmvk · 2021-01-26T05:57:29Z

@oranagra I will follow up with a doc soon.

Documentation changes corresponding to redis/redis#8327.

Documentation changes corresponding to PR redis/redis#8327.

nmvk · 2021-01-26T18:25:11Z

Document PR - redis/redis-doc#1501

Documentation changes corresponding to PR redis/redis#8327.

…redis#8357) This commit adds tests to make sure that relative and absolute expire commands are propagated as is to replicas and stop any future attempt to change that without a proper discussion. see redis#8327 and redis#5171 Additionally it slightly improve the AOF test that tests the opposite (always propagating absolute times), by covering more commands, and shaving 2 seconds from the test time.

This commit introduces two new command and two options for an existing command GETEX <key> [PERSIST][EX seconds][PX milliseconds] [EXAT seconds-timestamp] [PXAT milliseconds-timestamp] The getexCommand() function implements extended options and variants of the GET command. Unlike GET command this command is not read-only. Only one of the options can be used at a given time. 1. PERSIST removes any TTL associated with the key. 2. EX Set expiry TTL in seconds. 3. PX Set expiry TTL in milliseconds. 4. EXAT Same like EX instead of specifying the number of seconds representing the TTL (time to live), it takes an absolute Unix timestamp 5. PXAT Same like PX instead of specifying the number of milliseconds representing the TTL (time to live), it takes an absolute Unix timestamp Command would return either the bulk string, error or nil. GETDEL <key> Would delete the key after getting. SET key value [NX] [XX] [KEEPTTL] [GET] [EX <seconds>] [PX <milliseconds>] [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] Two new options added here are EXAT and PXAT Key implementation notes - `SET` with `PX/EX/EXAT/PXAT` is always translated to `PXAT` in `AOF`. When relative time is specified (`PX/EX`), replication will always use `PX`. - `setexCommand` and `psetexCommand` would no longer need translation in `feedAppendOnlyFile` as they are modified to invoke `setGenericCommand ` with appropriate flags which will take care of correct AOF translation. - `GETEX` without any optional argument behaves like `GET`. - `GETEX` command is never propagated, It is either propagated as `PEXPIRE[AT], or PERSIST`. - `GETDEL` command is propagated as `DEL` - Combined the validation for `SET` and `GETEX` arguments. - Test cases to validate AOF/Replication propagation

madolson added state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository labels Jan 13, 2021

madolson linked an issue Jan 13, 2021 that may be closed by this pull request

Feature new command GETEX #2762

Closed

madolson removed a link to an issue Jan 13, 2021

Feature new command GETEX #2762

Closed

madolson reviewed Jan 14, 2021

View reviewed changes

src/t_string.c Outdated Show resolved Hide resolved

nmvk force-pushed the getextended branch from 9a6cb13 to 30d26f7 Compare January 14, 2021 01:22

nmvk requested a review from madolson January 14, 2021 01:28

madolson linked an issue Jan 18, 2021 that may be closed by this pull request

Feature new command GETEX #2762

Closed

madolson added the approval-needed Waiting for core team approval to be merged label Jan 18, 2021

madolson previously approved these changes Jan 18, 2021

View reviewed changes

oranagra reviewed Jan 18, 2021

View reviewed changes

oranagra mentioned this pull request Jan 18, 2021

Add tests to make sure that relative EXPIRE is propagated to replicas #8357

Merged

nmvk dismissed madolson’s stale review via b230e18 January 19, 2021 06:45

nmvk force-pushed the getextended branch from b230e18 to a80fc97 Compare January 19, 2021 08:09

nmvk requested review from madolson and oranagra January 19, 2021 08:17

oranagra reviewed Jan 19, 2021

View reviewed changes

src/aof.c Outdated Show resolved Hide resolved

src/aof.c Outdated Show resolved Hide resolved

src/t_string.c Outdated Show resolved Hide resolved

src/t_string.c Outdated Show resolved Hide resolved

src/aof.c Outdated Show resolved Hide resolved

src/aof.c Outdated Show resolved Hide resolved

nmvk added 3 commits January 20, 2021 00:42

Added new tests for GETEX/SET PXAT/EXAT

4ba63a7

Replication and AOF test coverage for different options of GETEX. Modified the existing expiration test to include SET PXAT/EXAT and GETEX. Added a test to ensure if GETEX is invoked with no arguments then not to write anything to AOF file.

nmvk force-pushed the getextended branch from a80fc97 to 4ba63a7 Compare January 20, 2021 08:43

nmvk requested a review from oranagra January 20, 2021 08:49

oranagra reviewed Jan 20, 2021

View reviewed changes

Fix comments and Test cases

943b543

Updated comments from PR review. Updated GETEX PERSIST propagate to replica test to include DEL case.

nmvk requested a review from oranagra January 20, 2021 17:12

nmvk requested review from madolson, oranagra and yossigo January 25, 2021 19:33

nmvk changed the title ~~GETEX and SET PXAT/EXAT~~ GETEX, GETDEL and SET PXAT/EXAT Jan 25, 2021

madolson reviewed Jan 26, 2021

View reviewed changes

src/t_string.c Outdated Show resolved Hide resolved

src/t_string.c Show resolved Hide resolved

tests/unit/type/string.tcl Outdated Show resolved Hide resolved

Fix PR Comments

ca7fb1d

Update the PR.

nmvk requested a review from madolson January 26, 2021 04:44

madolson reviewed Jan 26, 2021

View reviewed changes

src/t_string.c Outdated Show resolved Hide resolved

Propagate as Unlink

8c7abe8

Propagate as UNLINK

nmvk requested a review from madolson January 26, 2021 05:27

madolson approved these changes Jan 26, 2021

View reviewed changes

oranagra approved these changes Jan 26, 2021

View reviewed changes

yossigo approved these changes Jan 26, 2021

View reviewed changes

nmvk pushed a commit to nmvk/redis-doc that referenced this pull request Jan 26, 2021

GETEX, GETDEL and SET PXAT/EXAT Documentation

4f1a568

Documentation changes corresponding to redis/redis#8327.

nmvk added a commit to nmvk/redis-doc that referenced this pull request Jan 26, 2021

GETEX, GETDEL and SET PXAT/EXAT Documentation

fe76521

Documentation changes corresponding to PR redis/redis#8327.

nmvk added a commit to nmvk/redis-doc that referenced this pull request Jan 26, 2021

GETEX, GETDEL and SET PXAT/EXAT Documentation

a6a5a0d

Documentation changes corresponding to PR redis/redis#8327.

nmvk mentioned this pull request Jan 26, 2021

GETEX, GETDEL and SET PXAT/EXAT Documentation redis/redis-doc#1501

Merged

oranagra merged commit 0367a80 into redis:unstable Jan 27, 2021

itamarhaber pushed a commit to redis/redis-doc that referenced this pull request Jan 28, 2021

GETEX, GETDEL and SET PXAT/EXAT Documentation (#1501)

3672323

Documentation changes corresponding to PR redis/redis#8327.

oranagra mentioned this pull request Jan 31, 2021

6.2 RC3 #8427

Merged

This was referenced Feb 2, 2021

Add support for GETEX, GETDEL commands redis/lettuce#1606

Closed

Add support for PXAT/EXAT arguments to SET command redis/lettuce#1607

Closed

enjoy-binbin mentioned this pull request Aug 6, 2021

[Feature request] TAKE - simple GET + DELETE #7324

Closed

soloestoy mentioned this pull request Aug 29, 2022

[NEW] zrank with score option #11111

Closed

GETEX, GETDEL and SET PXAT/EXAT #8327

GETEX, GETDEL and SET PXAT/EXAT #8327

Uh oh!

Conversation

nmvk commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

madolson commented Jan 18, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soloestoy commented Jan 19, 2021

Uh oh!

oranagra commented Jan 19, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jan 20, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yossigo commented Jan 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jan 26, 2021

Uh oh!

nmvk commented Jan 26, 2021

Uh oh!

nmvk commented Jan 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nmvk commented Jan 13, 2021 •

edited

Loading