[go: up one dir, main page]

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

Redis slaves, while not allowed to expire keys without master input, should reply to clients consistently with the key expire information. #187

Closed
antirez opened this issue Nov 12, 2011 · 5 comments

Comments

@antirez
Copy link
Contributor
antirez commented Nov 12, 2011

Facts:

  • Redis slaves don't expire keys actively, since the wait for a DEL form the master.
  • Expiration was centralized in order to allow clients to write to keys with an expire set without time dependent inconsistencies in 2.2.
  • Redis master will expire keys in two ways: incrementally in background, sampling random keys. In a lazy way when the key is accessed, so that even if the key was not already expired in the Redis cron we expire it as soon as a client will try to access it.

The above points are good and make the Redis expire semantic simpler from the point of view of the append only file and replication.

However consider this: a Redis master could have 50 million keys with an expire set, however all the keys but just one have the expire set in the future. The only key with a short expire is unlikely to be found by random sampling, but if a client will access this key, the key will be evicted by the lookupKey...() function and everything will work as expected.

But if a client will try to access this key in a slave it will found it existing: lookupKey...() can't expire keys in a slave.

How to fix that?

The obvious way to fix this issue is to make the slave more aware of what it is doing. It could simply compute if the key is expired so that lookupKey...() will return NULL and the client will be non existing from the point of view of the client request, even if the salve will not evict the key form the database, waiting for the DEL from the master instance in order to do so. This way we don't have time dependent behaviors when the slave receives, for example, an SUNIONSTORE command that has that key on its arguments (the key may not be already expired on the master since the two host timers may not be perfectly synchronized, or because there is a delay in the replication link, and so forth).

This fix is just one line and trivial, but there are a few problems with it:

  1. Lookup functions are also called by commands doing work that depends on the fact the key exists or not to create new keys or new state inside the database, like SUNIONSTORE. This would require a new lookupKey... interface where the caller can say if the key is going to be used just for a read only command or to use the key content to alter the state of the data set.
  2. From the point of view of Lua scripting the behavior should always be that keys exist if not evicted with an explicit DEL from the master.

This requires a refactoring of the internal API, so for now it is considered a post-3.0 stuff, but it is better to write it down and think about ways to resolve this issue in the future.

@shafreeck
Copy link
shafreeck commented Dec 8, 2016

@antirez
This has been fixed by #1768 , maybe it's time to close this issue.

@orlra
Copy link
orlra commented Jan 30, 2018

@shafreeck This commit never hit master.
problem exists. We still get same problems with slaves.
up to 2.8.X:
GET-> old_value
Exists -> 1
from 3.0.5 -> 4.0.6
GET-> null
Exists -> 1
if you want to replicate:

  1. create master--slave nodes.
  2. insert 10k items with TTL max (intended to lower chance for garbage collection, might need a bit more than 10k depending on settings)
  3. insert your key with TTL ~10 sec
  4. ping slaves with exists and get functions.

@wasikuss
Copy link
wasikuss commented May 8, 2018

same situation can be reproduced easier by:

  1. create master-slave nodes
  2. insert one key witch expire setex key 5 val
  3. kill master and wait 5s
  4. slave will return
    • ttl key -> -2
    • exists key -> 1
    • get key -> null

@AD7six
Copy link
AD7six commented Jan 25, 2019

For info this step is not necessary IME: kill master and wait 5s

On a busy redis cluster, this was my experience (redis_version:4.0.8):

primary> SET mykey "Hello"
OK
primary> EXPIRE mykey 10
(integer) 1
replica> get mykey
"Hello"
replica> ttl mykey
(integer) 10
replica> exists mykey
(integer) 1
 
replica> get mykey
"Hello"
replica> ttl mykey
(integer) 0
replica> exists mykey
(integer) 1
 
replica> get mykey
(nil)
replica> ttl mkey
(integer) -2
replica> exists mykey
(integer) 1

@oranagra
Copy link
Member

indeed this was solved long ago.
initially handled in #1768, then the EXISTS command issue was handled in #5021
and most recently SUNIONSTORE and others in #9572

The only problem mentioned here that's not already solved in the Lua issue, but that's in some way solved by b089ba9 and other changes in that area

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants