diff --git a/src/cluster.c b/src/cluster.c index 490f25b5b..54e58e28b 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -6702,7 +6702,7 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in * slot migration, the channel will be served from the source * node until the migration completes with CLUSTER SETSLOT * NODE . */ - int flags = LOOKUP_NOTOUCH | LOOKUP_NOSTATS | LOOKUP_NONOTIFY; + int flags = LOOKUP_NOTOUCH | LOOKUP_NOSTATS | LOOKUP_NONOTIFY | LOOKUP_NOEXPIRE; if ((migrating_slot || importing_slot) && !is_pubsubshard) { if (lookupKeyReadWithFlags(&server.db[0], thiskey, flags) == NULL) missing_keys++; diff --git a/src/db.c b/src/db.c index 36de2aa0a..3fd4938af 100644 --- a/src/db.c +++ b/src/db.c @@ -41,7 +41,11 @@ * C-level DB API *----------------------------------------------------------------------------*/ -int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired); +/* Flags for expireIfNeeded */ +#define EXPIRE_FORCE_DELETE_EXPIRED 1 +#define EXPIRE_AVOID_DELETE_EXPIRED 2 + +int expireIfNeeded(redisDb *db, robj *key, int flags); int keyIsExpired(redisDb *db, robj *key); /* Update LFU when an object is accessed. @@ -72,6 +76,8 @@ void updateLFU(robj *val) { * LOOKUP_NOSTATS: Don't increment key hits/misses counters. * LOOKUP_WRITE: Prepare the key for writing (delete expired keys even on * replicas, use separate keyspace stats and events (TODO)). + * LOOKUP_NOEXPIRE: Perform expiration check, but avoid deleting the key, + * so that we don't have to propagate the deletion. * * Note: this function also returns NULL if the key is logically expired but * still existing, in case this is a replica and the LOOKUP_WRITE is not set. @@ -92,8 +98,12 @@ robj *lookupKey(redisDb *db, robj *key, int flags) { * command, since the command may trigger events that cause modules to * perform additional writes. */ int is_ro_replica = server.masterhost && server.repl_slave_ro; - int force_delete_expired = flags & LOOKUP_WRITE && !is_ro_replica; - if (expireIfNeeded(db, key, force_delete_expired)) { + int expire_flags = 0; + if (flags & LOOKUP_WRITE && !is_ro_replica) + expire_flags |= EXPIRE_FORCE_DELETE_EXPIRED; + if (flags & LOOKUP_NOEXPIRE) + expire_flags |= EXPIRE_AVOID_DELETE_EXPIRED; + if (expireIfNeeded(db, key, expire_flags)) { /* The key is no longer valid. */ val = NULL; } @@ -1657,13 +1667,17 @@ int keyIsExpired(redisDb *db, robj *key) { * * On replicas, this function does not delete expired keys by default, but * it still returns 1 if the key is logically expired. To force deletion - * of logically expired keys even on replicas, set force_delete_expired to - * a non-zero value. Note though that if the current client is executing + * of logically expired keys even on replicas, use the EXPIRE_FORCE_DELETE_EXPIRED + * flag. Note though that if the current client is executing * replicated commands from the master, keys are never considered expired. * + * On the other hand, if you just want expiration check, but need to avoid + * the actual key deletion and propagation of the deletion, use the + * EXPIRE_AVOID_DELETE_EXPIRED flag. + * * The return value of the function is 0 if the key is still valid, * otherwise the function returns 1 if the key is expired. */ -int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired) { +int expireIfNeeded(redisDb *db, robj *key, int flags) { if (!keyIsExpired(db,key)) return 0; /* If we are running in the context of a replica, instead of @@ -1681,9 +1695,14 @@ int expireIfNeeded(redisDb *db, robj *key, int force_delete_expired) { * expired. */ if (server.masterhost != NULL) { if (server.current_client == server.master) return 0; - if (!force_delete_expired) return 1; + if (!(flags & EXPIRE_FORCE_DELETE_EXPIRED)) return 1; } + /* In some cases we're explicitly instructed to return an indication of a + * missing key without actually deleting it, even on masters. */ + if (flags & EXPIRE_AVOID_DELETE_EXPIRED) + return 1; + /* If clients are paused, we keep the current dataset constant, * but return to the client what we believe is the right state. Typically, * at the end of the pause we will properly expire the key OR we will diff --git a/src/server.h b/src/server.h index f33eaa5ce..bac67fe97 100644 --- a/src/server.h +++ b/src/server.h @@ -3099,6 +3099,7 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, #define LOOKUP_NONOTIFY (1<<1) /* Don't trigger keyspace event on key misses. */ #define LOOKUP_NOSTATS (1<<2) /* Don't update keyspace hits/misses counters. */ #define LOOKUP_WRITE (1<<3) /* Delete expired keys even in replicas. */ +#define LOOKUP_NOEXPIRE (1<<4) /* Avoid deleting lazy expired keys. */ void dbAdd(redisDb *db, robj *key, robj *val); int dbAddRDBLoad(redisDb *db, sds key, robj *val); diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 2dcccb8cf..f6b04c89c 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -95,6 +95,7 @@ set ::all_tests { unit/violations unit/replybufsize unit/cluster-scripting + unit/cluster/misc } # Index to the next test to run in the ::all_tests list. set ::next_test 0 diff --git a/tests/unit/cluster/misc.tcl b/tests/unit/cluster/misc.tcl new file mode 100644 index 000000000..35308b81a --- /dev/null +++ b/tests/unit/cluster/misc.tcl @@ -0,0 +1,16 @@ +start_cluster 2 2 {tags {external:skip cluster}} { + test {Key lazy expires during key migration} { + R 0 DEBUG SET-ACTIVE-EXPIRE 0 + + set key_slot [R 0 CLUSTER KEYSLOT FOO] + R 0 set FOO BAR PX 10 + set src_id [R 0 CLUSTER MYID] + set trg_id [R 1 CLUSTER MYID] + R 0 CLUSTER SETSLOT $key_slot MIGRATING $trg_id + R 1 CLUSTER SETSLOT $key_slot IMPORTING $src_id + after 11 + assert_error {ASK*} {R 0 GET FOO} + R 0 ping + } {PONG} +} +