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.

Sign up for GitHub

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

Incr expired_keys if the expiration time is already expired #1517

Merged
merged 2 commits into from
Jan 16, 2025
Merged

Incr expired_keys if the expiration time is already expired #1517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aafe746
add expired_keys statistic if the expiration time is already expired
RayaCoo Jan 6, 2025
11348fe
make test more stable
RayaCoo Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/db.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1860,7 +1860,8 @@ void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj) {
robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del;
rewriteClientCommandVector(c, 2, aux, keyobj);
signalModifiedKey(c, c->db, keyobj);
notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyobj, c->db->id);
notifyKeyspaceEvent(NOTIFY_EXPIRED, "expired", keyobj, c->db->id);
Copy link
Contributor

@zuiderkwast zuiderkwast Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between expire and expired event? I think the difference between "expire" and "expired" is clear in the docs.

In the docs I can find for "expired":

  • Every time a key with a time to live associated is removed from the data set because it expired, an expired event is generated.

and for "expire" event:

  • EXPIRE and all its variants (PEXPIRE, EXPIREAT, PEXPIREAT) generate an expire event when called with a positive timeout (or a future timestamp). Note that when these com‐
    mands are called with a negative timeout value or timestamp in the past, the key is deleted and only a del event is generated instead.

This seems to me like a bug fix. A key expiration is supposed to trigger an expired event. A del event seems like a bug. Or am I missing anything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can find "expired" in active expire path: activeExpireCycleTryExpire -> deleteExpiredKeyAndPropagate -> deleteExpiredKeyAndPropagateWithDictIndex

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Yes I found it.

Shall we postpone the breaking change to 9.0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think 8.1 is ok, it's a very very small change maybe even not breaking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do it in 8.1, we can describe it as a bug fix. It looks better than a breaking change in a minor version.

But let's be sure we know what we are doing. We have done some mistakes before such as #1537 and #1228. We often think that small things like that will not be a problem.

@madolson said

Even if the new behavior is more correct, it's such a long standing behavior someone might have coded around it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very sure what it is : )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very sure what it is : )

What is it exactly?

If it's a breaking change, we shouldn't release it in a minor version.

  • Breaking change → release in 9.0.
  • Not breaking change → release in 8.1 and remove the breaking-change label.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bugfix, but it might also introduce a breaking change. These two are not mutually exclusive, and I don't understand why it has to be an either-or situation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we use semantic versioning. It's the only reason.

Major version must be incremented if we introduce backward incompatible API changes.

If the public documented API is not changed, then we are just fixing incorrect behaviour. Then a patch release is enough.

https://semver.org

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this PR doesn't introduce API changes, I removed the breaking-change flag, and 8.1 is the right way

server.stat_expiredkeys++;
}

/* Propagate an implicit key deletion into replicas and the AOF file.
Expand Down
25 changes: 25 additions & 0 deletions tests/unit/expire.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,31 @@ start_server {tags {"expire"}} {
list $e $f
} {somevalue {}}

test {EXPIRE / EXPIREAT / PEXPIRE / PEXPIREAT Expiration time is already expired} {
r flushall
r config resetstat

r set x somevalue
r expire x -1
assert_equal {0} [r exists x]
assert_equal {1} [s expired_keys]

r set x somevalue
r expireat x [expr [clock seconds] - 1]
assert_equal {0} [r exists x]
assert_equal {2} [s expired_keys]

r set x somevalue
r pexpire x -1000
assert_equal {0} [r exists x]
assert_equal {3} [s expired_keys]

r set x somevalue
r pexpireat x [expr [clock milliseconds] - 1000]
assert_equal {0} [r exists x]
assert_equal {4} [s expired_keys]
}

test {TTL returns time to live in seconds} {
r del x
r setex x 10 somevalue
Expand Down
11 changes: 11 additions & 0 deletions tests/unit/pubsub.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,17 @@ start_server {tags {"pubsub network"}} {
$rd1 close
}

test "Keyspace notification: expired event (Expiration time is already expired)" {
r config set notify-keyspace-events Ex
r del foo
set rd1 [valkey_deferring_client]
assert_equal {1} [psubscribe $rd1 *]
r set foo 1
r expire foo -1
assert_equal "pmessage * __keyevent@${db}__:expired foo" [$rd1 read]
$rd1 close
}

test "Keyspace notifications: evicted events" {
r config set notify-keyspace-events Ee
r config set maxmemory-policy allkeys-lru
Expand Down
Loading