@soloestoy FYI

speaking of which, maybe other commands should not affect access time? OBJECT?
@redis/core-team

I don't object to cleaning the stables and locating more commands that violate this principle, but unless introduced by accident like in this case, we have to consider the (skewed) use case of someone actually doing OBJECT to touch a key. For these we may want to wait for 6.2.

Have a question about LOOKUP_NOTOUCH flag usage, I just grep it and found all commands now use it: TTL/SCAN/DEBUG and EXISTS and maybe module open a key with REDISMODULE_OPEN_KEY_NOTOUCH.

I understand the DEBUG command should not touch access time, but others seem like normal commands and users use them not for debugging, so I'm confused when to use LOOKUP_NOTOUCH flag.

@soloestoy yes, that's a grey area in Redis... Maybe it's time to formalize it: I suggest that any command that doesn't access the data of a key should not affect LRU/LFU. Except TOUCH.

I think SCAN (checking that the key didn't expire) and EXIST are kinda obvious that they should not touch the LRU.
OBJECT and DEBUG are introspection commands, so they shouldn't touch the LRU either.

Note that OBJECT uses dictFind right now, but i think it better use lookupKey(LOOKUP_NOTOUCH) (if they key already expired it should not be handled).

I think DEBUG DIGEST-VALUE should be changed to be like DEBUG OBJECT, and use dictFind directly.

Not sure about TTL and EXPIRE (which i suppose should behave similarly in that respect), maybe TTL should be changed to touch the LRU.

BTW, besides the access time, if key doesn't exist lookupKeyReadWithFlags would call notifyKeyspaceEvent to send keymiss message, so if we take a command as introspection, I think we should not use lookupKeyReadWithFlags.

So we can add a flag named LOOKUP_NONOTIFY and use it in OBJECT (in addition to the changes i listed in my previous message).

We can also add a LOOKUP_NOEXPIRE and then we can let DEBUG command use lookupKey (with all 3 flags set), but i'm not sure what's the point in that.

@redis/core-team pushed another commit fixing DEBUG digest-value (to handle expired keys) and OBJECT (to avoid handling expired keys)

the only thing left to decide is whether EXPIRE command(s) affect LRU/LFU
my gut says "yes" but i don't have any justification.. maybe except that some users can actually be affected if we decide that EXPIRE doesn't change access time

I agree with @guybe7's gut

[deleted my last message since it stated incorrect facts, here's the updated one]

i tend to agree.. (keep EXPIRE as it is, which is to "thouch" the LRU/LFU).
let's try to group all commands that access keys into 4 categories

1. commands reading from the key data
2. commands writing to the key data
3. commands reading from the key metadata (EXISTS, TYPE, TTL, OBJECT ENCODING, OBJECT IDLETIME) - note that there was a test that confirms this for TTL and TYPE, but not for EXISTS.
4. commands writing to the key metadata (EXPIRE)

• it's clear that groups 1 and 2 should "touch" the key LRU/LFU.
• in the past (and again after the fix this PR makes to an accidental change), group 3 (TYPE, EXIST, TTL) would not "touch" the LRU.
• regarding group 4 (the EXPIRE), i have mixed feelings. till now it did "touch" the LRU, and i'm not sure i wanna change that.

So this means that if we agree, this PR is ready to be merged.
the summary of the changes is:

• EXIST changed to touch the LRU (revert of an unintended change)
• DEBUG DIGEST-VALUE changed to handle expired keys (like DEBUG OBJECT)
• OBJECT changed to avoid handling logically expired keys

@redis/core-team please approve. or state what you think about EXPIRE's behavior.

Agreed about not changing the existing touchy EXPIRE for now.

Note: the commit comment that was merged was wrong, in the sense that it indicated that the effect of the change in OBJECT is only on replica, but in fact the change affects master too.