Skip to content

Commit

Permalink
use startEvictionTimeProc() in config set maxmemory (redis#10019)
Browse files Browse the repository at this point in the history
This would mean that the effects of `CONFIG SET maxmemory` may not be visible once the command returns.
That could anyway happen since incremental eviction was added in redis 6.2 (see redis#7653)

We do this to fix one of the propagation bugs about eviction see redis#9890 and redis#10014.
  • Loading branch information
soloestoy authored Jan 4, 2022
1 parent 87789fa commit 2e1979a
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 9 deletions.
2 changes: 1 addition & 1 deletion src/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -2252,7 +2252,7 @@ static int updateMaxmemory(const char **err) {
if (server.maxmemory < used) {
serverLog(LL_WARNING,"WARNING: the new maxmemory value set via CONFIG SET (%llu) is smaller than the current memory usage (%zu). This will result in key eviction and/or the inability to accept new write commands depending on the maxmemory-policy.", server.maxmemory, used);
}
performEvictions();
startEvictionTimeProc();
}
return 1;
}
Expand Down
14 changes: 9 additions & 5 deletions src/evict.c
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,14 @@ static int evictionTimeProc(
return AE_NOMORE;
}

void startEvictionTimeProc(void) {
if (!isEvictionProcRunning) {
isEvictionProcRunning = 1;
aeCreateTimeEvent(server.el, 0,
evictionTimeProc, NULL, NULL);
}
}

/* Check if it's safe to perform evictions.
* Returns 1 if evictions can be performed
* Returns 0 if eviction processing should be skipped
Expand Down Expand Up @@ -712,11 +720,7 @@ int performEvictions(void) {
* memory, don't want to spend too much time here. */
if (elapsedUs(evictionTimer) > eviction_time_limit_us) {
// We still need to free memory - start eviction timer proc
if (!isEvictionProcRunning) {
isEvictionProcRunning = 1;
aeCreateTimeEvent(server.el, 0,
evictionTimeProc, NULL, NULL);
}
startEvictionTimeProc();
break;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -3044,7 +3044,7 @@ unsigned long LFUDecrAndReturn(robj *o);
#define EVICT_RUNNING 1
#define EVICT_FAIL 2
int performEvictions(void);

void startEvictionTimeProc(void);

/* Keys hashing / comparison functions for dict.c hash tables. */
uint64_t dictSdsHash(const void *key);
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/moduleapi/propagate.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ tags "modules" {

# Note whenever there's double notification: SET with EX issues two separate
# notifications: one for "set" and one for "expire"
# "config set" should not be here, see https://github.com/redis/redis/issues/10014
# Note that although CONFIG SET maxmemory is called in this flow (see issue #10014),
# eviction will happen and will not induce propagation of the CONFIG command (see #10019).
assert_replication_stream $repl {
{select *}
{multi}
Expand All @@ -198,7 +199,6 @@ tags "modules" {
{del asdf*}
{incr notifications}
{del asdf*}
{config set maxmemory 1}
{multi}
{incr notifications}
{set asdf4 4}
Expand Down

0 comments on commit 2e1979a

Please sign in to comment.