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

Support EXAT/PEXAT option in the set command #901

Merged
merged 7 commits into from
Sep 28, 2022
Merged

Support EXAT/PEXAT option in the set command #901

merged 7 commits into from
Sep 28, 2022

Conversation

ellutionist
Copy link
Contributor

PR to solve issue #891

git-hulk
git-hulk reviewed Sep 20, 2022
View reviewed changes
src/slot_migrate.cc Outdated Show resolved Hide resolved
xiaobiaozhao
xiaobiaozhao reviewed Sep 22, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
tanruixiang
tanruixiang reviewed Sep 23, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Sep 26, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Sep 26, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
PragmaTwice
PragmaTwice reviewed Sep 26, 2022
View reviewed changes
src/redis_cmd.cc Outdated Show resolved Hide resolved
@git-hulk git-hulk changed the title fix: non positive ttl in slot migration Support EXAT/PEXAT option in the set command Sep 26, 2022
@git-hulk
Copy link
Member

@ellutionist Can you help to resolve the conflict in redis_cmd.cc

@ellutionist
Copy link
Contributor Author

ellutionist commented Sep 26, 2022 •
edited
Loading

@ellutionist Can you help to resolve the conflict in redis_cmd.cc

@git-hulk Conflict resolved.

git-hulk
git-hulk previously approved these changes Sep 27, 2022
View reviewed changes
@git-hulk git-hulk requested review from PragmaTwice, tanruixiang and xiaobiaozhao and removed request for tanruixiang September 27, 2022 05:05
tanruixiang
tanruixiang requested changes Sep 27, 2022
View reviewed changes
Copy link
Member

@tanruixiang tanruixiang left a comment

Choose a reason for hiding this comment

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

HI. Thank you very much for your contribution. The code could be a little more concise.

src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
src/redis_cmd.cc Outdated Show resolved Hide resolved
@tanruixiang
Copy link
Member

@ellutionist Thank you very much for your contribution. LGTM. Do you have time to add a unit test for cpp? It should be in t_string_test.cc.

@ellutionist
Copy link
Contributor Author

@ellutionist Thank you very much for your contribution. LGTM. Do you have time to add a unit test for cpp? It should be in t_string_test.cc.

@tanruixiang Since this PR does not bring any new interface like SetEX or MSet to the class Redis::String, I cannot see any proper entry point for cpp unit test. Any good idea?

@tanruixiang
Copy link
Member

@tanruixiang Since this PR does not bring any new interface like SetEX or MSet to the class Redis::String, I cannot see any proper entry point for cpp unit test. Any good idea?

Sorry, I revisited the code. You're right. I guess tcl's tests are enough. Thank you very much for your contribution.

@git-hulk
Copy link
Member

Many thanks for your contribution, will merge if there are no more comments by tomorrow. cc @PragmaTwice @caipengbo

@git-hulk git-hulk merged commit b94ea81 into apache:unstable Sep 28, 2022
@tisonkun tisonkun mentioned this pull request Oct 4, 2022
Closed
2 tasks
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

@xiaobiaozhao xiaobiaozhao xiaobiaozhao approved these changes

@tanruixiang tanruixiang tanruixiang approved these changes

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

@PragmaTwice PragmaTwice Awaiting requested review from PragmaTwice

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

Successfully merging this pull request may close these issues.
5 participants
@ellutionist @git-hulk @tanruixiang @PragmaTwice @xiaobiaozhao