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.

Sign up for GitHub

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

Jump to bottom

Implement the GETEX command #961

Merged
merged 26 commits into from
Oct 12, 2022
Merged

Implement the GETEX command #961

merged 26 commits into from
Oct 12, 2022

Conversation

maochongxin
Copy link
Contributor

@maochongxin maochongxin commented Oct 8, 2022 •
edited by git-hulk
Loading

This closes #960

Has passed the string.tcl test

➜  redis git:(6.2) ✗ tclsh tests/test_helper.tcl --host 0.0.0.0 --port 6666
Cleanup: may take some time... OK
Starting test server at port 21079
[ready]: 37258
Testing unit/type/string
[ok]: SET and GET an item
[ok]: SET and GET an empty item
[ok]: Very big payload in GET/SET
[ok]: Very big payload random access
[ok]: SET 10000 numeric keys and access all them in reverse order
[ok]: SETNX target key missing
[ok]: SETNX target key exists
[ok]: SETNX against not-expired volatile key
[ok]: SETNX against expired volatile key
[ok]: GETEX EX option
[ok]: GETEX PX option
[ok]: GETEX EXAT option
[ok]: GETEX PXAT option
[ok]: GETEX PERSIST option
[ok]: GETEX no option
[ok]: GETEX syntax errors
[ok]: GETEX no arguments
[ok]: GETDEL command

@maochongxin maochongxin mentioned this pull request Oct 8, 2022
Closed
2 tasks
@maochongxin maochongxin marked this pull request as draft October 8, 2022 11:40
@git-hulk
Copy link
Member

git-hulk commented Oct 8, 2022

cool, thanks for your contribution.

@maochongxin Can we put the ttl argument parse into a single function? so that we can avoid duplicate codes, the expire argument can be translated into ttl as well.

@maochongxin
Copy link
Contributor Author

cool, thanks for your contribution.

@maochongxin Can we put the ttl argument parse into a single function? so that we can avoid duplicate codes, the expire argument can be translated into ttl as well.

Of course, I'm going to optimize it

@maochongxin
Copy link
Contributor Author

cool, thanks for your contribution.

@maochongxin Can we put the ttl argument parse into a single function? so that we can avoid duplicate codes, the expire argument can be translated into ttl as well.

@git-hulk Complete the refactoring, but Is it waiting for #963 conclusion

@PragmaTwice
Copy link
Member

PragmaTwice commented Oct 9, 2022 •
edited
Loading 

SET key value [NX | XX] [GET] [EX seconds | PX milliseconds |
  EXAT unix-time-seconds | PXAT unix-time-milliseconds | KEEPTTL]

GETEX key [EX seconds | PX milliseconds | EXAT unix-time-seconds |
  PXAT unix-time-milliseconds | PERSIST]

I think we should notice their difference: there is no PERSIST in SET, and no NX, XX in GETEX, hence we should give a syntax error to user e.g. while encountering NX in GETEX.

So I do not think the whole parsing procedure for SET and GETEX can be put in one function (or you can try it, but I think it might make the logic more complicated), you could just unify the EX/PX/EXAT/PXAT part.

PragmaTwice
PragmaTwice reviewed Oct 9, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Oct 9, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Oct 9, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
@maochongxin maochongxin marked this pull request as ready for review October 10, 2022 10:00
PragmaTwice
PragmaTwice reviewed Oct 10, 2022
View reviewed changes
src/util.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Oct 10, 2022
View reviewed changes
src/util.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Oct 10, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Oct 10, 2022
View reviewed changes
src/util.cc Outdated Show resolved Hide resolved
@maochongxin @PragmaTwice
Update src/util.cc
40164c1 
Co-authored-by: Twice <twice@apache.org>
PragmaTwice
PragmaTwice reviewed Oct 10, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Oct 10, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
@git-hulk git-hulk changed the title impl GETEX Implement the GETEX command Oct 11, 2022
git-hulk
git-hulk previously approved these changes Oct 11, 2022
View reviewed changes
@git-hulk git-hulk requested a review from PragmaTwice October 11, 2022 12:11
PragmaTwice
PragmaTwice reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@PragmaTwice PragmaTwice left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems these TCL tests are migrated to golang, so maybe we should add these GetEX tests in go test cases again. cc @maochongxin

@maochongxin
Copy link
Contributor Author

It seems these TCL tests are migrated to golang, so maybe we should add these GetEX tests in go test cases again. cc @maochongxin

Ok, let me add it

@maochongxin
Copy link
Contributor Author

It seems these TCL tests are migrated to golang, so maybe we should add these GetEX tests in go test cases again. cc @maochongxin

Ok, let me add it

done

PragmaTwice
PragmaTwice approved these changes Oct 12, 2022
View reviewed changes
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your contriubution!

@PragmaTwice PragmaTwice requested a review from git-hulk October 12, 2022 03:43
@PragmaTwice
Copy link
Member

Just for record: https://github.com/apache/incubator-kvrocks/actions/runs/3231715201/jobs/5291557196

FAIL: TestString/Extended_SET_EXAT_option

@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit ebb5629 into apache:unstable Oct 12, 2022
@git-hulk
Copy link
Member

submitted the doc updated PR: apache/kvrocks-website#19

@maochongxin maochongxin deleted the patch-1 branch November 15, 2022 08:22
enjoy-binbin added a commit to enjoy-binbin/kvrocks that referenced this pull request Jul 5, 2023
@enjoy-binbin
Fix GETEX not checking wrong type error causing key overwriting
5689f3b 
This bug causes keys to be overwritten:
```
127.0.0.1:6666> lpush foo bar
(integer) 1
127.0.0.1:6666> type foo
list
127.0.0.1:6666> getex foo
""
127.0.0.1:6666> type foo
string
```

The reason is that we did not return early for wrong type
error and then the code overwrites it as a string key.
We should throw a wrong type error in this case:
```
127.0.0.1:6666> lpush foo bar
(integer) 1
127.0.0.1:6666> getex foo
(error) ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value
127.0.0.1:6666> type foo
list
```

GETEX was added in apache#961, look like this bug has existed since
the command was added.
@enjoy-binbin enjoy-binbin mentioned this pull request Jul 5, 2023
Merged
enjoy-binbin added a commit to enjoy-binbin/kvrocks that referenced this pull request Aug 7, 2023
@enjoy-binbin
CAS command supports EXAT and PXAT options
763b6c1 
Since we have already implemented these options
in SET / GETEX, the change is very simple.

Some ref links:
- CAS was added in apache#415
- SET EXAP / PXAT options was added in apache#901
- GETEX was added in apache#961
@enjoy-binbin enjoy-binbin mentioned this pull request Aug 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PragmaTwice PragmaTwice PragmaTwice approved these changes

@git-hulk git-hulk git-hulk approved these changes

@caipengbo caipengbo Awaiting requested review from caipengbo

@ShooterIT ShooterIT Awaiting requested review from ShooterIT

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support GetEX command
5 participants
@maochongxin @git-hulk @PragmaTwice @ShooterIT @caipengbo