-
Notifications
You must be signed in to change notification settings - Fork 23.5k
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
Improve expire consistency on slaves #1768
Comments
Fixed in 06e76bc. |
Slaves key expire is orchestrated by the master. Sometimes the master will send the synthesized DEL to expire keys on the slave with a non trivial delay (when the key is not accessed, only the incremental expiry algorithm will expire it in background). During that time, a key is logically expired, but slaves still return the key if you GET (or whatever) it. This is a bad behavior. However we can't simply trust the slave view of the key, since we need the master to be able to send write commands to update the slave data set, and DELs should only happen when the key is expired in the master in order to ensure consistency. However 99.99% of the issues with this behavior is when a client which is not a master sends a read only command. In this case we are safe and can consider the key as non existing. This commit does a few changes in order to make this sane: 1. lookupKeyRead() is modified in order to return NULL if the above conditions are met. 2. Calls to lookupKeyRead() in commands actually writing to the data set are repliaced with calls to lookupKeyWrite(). There are redundand checks, so for example, if in "2" something was overlooked, we should be still safe, since anyway, when the master writes the behavior is to don't care about what expireIfneeded() returns. This commit is related to #1768, #1770, #2131.
I just checked that the issue persists on 3.04(stable) version. Checked the source, patch was never merged. Just wondering if we are actually planning to resolve the issue. Logically slaves are not consistent with master which is quite dangerous. |
There is some chance that it will be merged? |
Hello, it was merged in the context of Redis 3.2 that will go RC before end of the year. No way we can put this into 3.0, too big semantical change. Thanks. |
p.s. Redis 3.2 branch name is |
Yes a big problem with limit explanation on documents. |
In order for Redis to ensure consistency between a master and its slaves, eviction of keys with an expire are managed by the master, which sends an explicit DEL to its slaves when the key gets actually removed.
This means that slaves are not able to directly expire keys, even if these keys are logically expired on the master side. So a GET that will return null in the master side, may return a stale value in the slave side.
This is a limit when using slaves in order to scale reads. Also the behavior does not require to be as global as it is currently. Normally slaves are configured in a read-only fashion, so they can only accept read commands from normal clients, and write commands from the master.
lookupKeyRead() may be modified in order to return NULL in the context of a slave, if the current client is not the master, even if it will not actually evict the key from the key space.
To implement this we could resort to two APIs:
A more in depth analysis is needed, but likely just using this simple checks we could be able to have a more sane semantics of read commands on slaves in a safe way.
The text was updated successfully, but these errors were encountered: