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

fix: fix bug in del function of redis_proxy #489

Merged
merged 14 commits into from
Mar 5, 2020

Conversation

levy5307
Copy link
Contributor

@levy5307 levy5307 commented Mar 3, 2020

What problem does this PR solve?

In redis_proxy,"OK" will be returned using del function to delete a key, If the function executes successfully. But interface definition of redis shows that it should return the number of keys which are deleted.
While, there are some difference between interface definition of redis and redis proxy of pegasus. Because whether the key is exist or not, pegasus will return "OK" to redis proxy, so we cann't distinguish whether the key is deleted or not. so in this case , we unity returns 1.

Redis documentation about del: https://redis.io/commands/del

DEL key [key ...]
Removes the specified keys. A key is ignored if it does not exist.
Return value
Integer reply: The number of keys that were removed.
Examples

redis> SET key1 "Hello"
"OK"
redis> SET key2 "World"
"OK"
redis> DEL key1 key2 key3
(integer) 2
redis> 

What is changed and how it works?

Change the return value from "OK" to the number of keys.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    1.start a redis proxy in c4tst-injection
    2.clone redis code, and build it.
    3.use command of ./redis_cli -h host -p port to connect with the redis proxy
    4.insert a k-v pair into redis proxy:
redis> SET key "value"
"OK"
  1. in pervious versionm, regardless of the key is exist or not, del the key, it returns "OK"
redis> DEL key "value"
"OK"

6.del the key which was inserted before in current version. It returns 1, not "OK".

redis> DEL key "value"
(integer) 1

7.del a key which is not exist in current version. It returns 1, also.

redis> DEL not_exist_key "value"
(integer) 1

Related changes

  • Need to cherry-pick to the release branch

acelyc111
acelyc111 previously approved these changes Mar 3, 2020
@neverchanje neverchanje added the component/rpoxy redis proxy for pegasus label Mar 4, 2020
@neverchanje
Copy link
Contributor

neverchanje commented Mar 5, 2020

Can you include the manual test in the PR description? This PR does not show any kind of test for us.

Like this:

Previous version

> redis del a
> ok

Current version

> redis del a a
> -1
> redis del a
> 1

@acelyc111 acelyc111 closed this Mar 5, 2020
@acelyc111 acelyc111 reopened this Mar 5, 2020
@neverchanje
Copy link
Contributor

I tried these commands on http://try.redis.io/. Deleting a non-existed key will return 0, but your version returns 1. This should be noted in our code.

> set a a
OK
> del a
(integer) 1
> del a
(integer) 0

@levy5307 levy5307 dismissed stale reviews from acelyc111 via 9757495 March 5, 2020 03:33
neverchanje
neverchanje previously approved these changes Mar 5, 2020
@levy5307 levy5307 merged commit 650a6f5 into apache:master Mar 5, 2020
@neverchanje neverchanje mentioned this pull request Mar 31, 2020
@levy5307 levy5307 deleted the redis_proxy_bug branch May 26, 2020 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.12.3 component/rpoxy redis proxy for pegasus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants