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.

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

[BUG] Flash Expiry Incorrect Logic #749

Open
keithchew opened this issue Nov 27, 2023 · 1 comment
Open

[BUG] Flash Expiry Incorrect Logic #749

keithchew opened this issue Nov 27, 2023 · 1 comment

Comments

@keithchew
Copy link

keithchew commented Nov 27, 2023

Describe the bug

With a new DB:

set test 222 EX 5

Logging the keys from expire.cpp, ie:

keys = db->getStorageCache()->getExpirationCandidates(ACTIVE_EXPIRE_CYCLE_LOOKUPS_PER_LOOP);
  for (std::string key : keys) {
      serverLog(LL_WARNING, "getExpirationCandidates [%s]", key.c_str()); <---- ADD THIS

The output will be:

43280:43377:M 27 Nov 2023 09:47:02.551 # getExpirationCandidates [test]
43280:43377:M 27 Nov 2023 09:47:02.651 # getExpirationCandidates [test]
43280:43377:M 27 Nov 2023 09:47:02.752 # getExpirationCandidates [test]

After 5s, the logs will stop, as the key has been removed from the expiry storage. All good.

Now, run this:

set test 222 EX 5
set test 222

After 5s, the logs keep outputting, ie the expiry entry is stuck.

...
43383:43420:M 27 Nov 2023 09:56:04.344 # getExpirationCandidates [test]
43383:43420:M 27 Nov 2023 09:56:04.445 # getExpirationCandidates [test]
43383:43420:M 27 Nov 2023 09:56:04.445 # getExpirationCandidates [test]
43383:43420:M 27 Nov 2023 09:56:04.545 # getExpirationCandidates [test]
43383:43420:M 27 Nov 2023 09:56:04.545 # getExpirationCandidates [test]
43383:43420:M 27 Nov 2023 09:56:04.646 # getExpirationCandidates [test]
43383:43420:M 27 Nov 2023 09:56:04.646 # getExpirationCandidates [test]
...

No matter what we do after this, eg

set test 222 EX 5
del test

The expiry entry is still stuck. It gets worse, looking at the logic in expire.cpp:

   keys = db->getStorageCache()->getExpirationCandidates(ACTIVE_EXPIRE_CYCLE_LOOKUPS_PER_LOOP);

In each loop, it will looking 64 keys to expire. If we repeat the process above for more than 64 keys, we will prevent the loop from removing new expiry entries!

@keithchew
Copy link
Author

I can confirm that there are (at least) 2 scenarios not handled by the current expiration logic:

  • the key is updated with a new expiry (later than current) or no expiry (ttl removed)
  • the key no longer exists

In both cases, the logic needs to remove the key from the expiry storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant