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

Closed
antirez opened this Issue May 21, 2014 · 6 comments

Comments

Projects
None yet
4 participants
@antirez
Owner

antirez commented May 21, 2014

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:

  • expireIfNeeded() in the context of slaves don't really try to evict expired keys, but just returns a return-value to signal if the key should be expired according to our local clock.
  • server.current_client could be used in order to test if the current client is our master.

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.

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Dec 10, 2014

Owner

Fixed in 06e76bc.

Owner

antirez commented Dec 10, 2014

Fixed in 06e76bc.

@antirez antirez closed this Dec 10, 2014

antirez added a commit that referenced this issue Dec 10, 2014

Better read-only behavior for expired keys in slaves.
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.
@mangatmodi

This comment has been minimized.

Show comment
Hide comment
@mangatmodi

mangatmodi Oct 1, 2015

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.

mangatmodi commented Oct 1, 2015

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.

@sirdawidd

This comment has been minimized.

Show comment
Hide comment
@sirdawidd

sirdawidd Oct 27, 2015

There is some chance that it will be merged?

sirdawidd commented Oct 27, 2015

There is some chance that it will be merged?

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Oct 27, 2015

Owner

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.

Owner

antirez commented Oct 27, 2015

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.

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Oct 27, 2015

Owner

p.s. Redis 3.2 branch name is testing.

Owner

antirez commented Oct 27, 2015

p.s. Redis 3.2 branch name is testing.

@zagfai

This comment has been minimized.

Show comment
Hide comment
@zagfai

zagfai Aug 15, 2016

Yes a big problem with limit explanation on documents.

zagfai commented Aug 15, 2016

Yes a big problem with limit explanation on documents.

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