Add CLIENT NO-TOUCH for clients to run commands without affecting LRU/LFU of keys #11483

CharlesChen888 · 2022-11-07T12:18:57Z

To resolve #11409

When no-touch mode is enabled, the client will not touch LRU/LFU of the keys it accesses, except when executing command TOUCH.
This allows inspecting or modifying the key-space without affecting their eviction.

Changes:

A command CLIENT NO-TOUCH ON|OFF to switch on and off this mode.
A client flag #define CLIENT_NOTOUCH (1ULL<<45), which can be shown with CLIENT INFO, by the letter "T" in the "flags" field.
Clear NO-TOUCH flag in clearClientConnectionState, which is used by RESET command and resetting temp clients used by modules.
Also clear NO-EVICT flag in clearClientConnectionState, this might have been an oversight, spotted by @madolson.
A test using DEBUG OBJECT command to verify that LRU stat is not touched when no-touch mode is on.

src/expire.c

src/db.c

zuiderkwast

Cool feature.

If found one more call to lookupKeyReadWithFlags without the NOTOUCH flag, in blocked.c, function handleClientsBlockedOnKeys:

            /* Serve clients blocked on the key. */
            robj *o = lookupKeyReadWithFlags(rl->db, rl->key, LOOKUP_NONOTIFY | LOOKUP_NOSTATS);

What do you think about this one? If the key just became ready, it must have just been written and therefore just touched, so touching it again doesn't hurt? But then we could as well add the NOTOUCH flag here...

madolson

Would you mind adding a simple test that it works? Probably in unit/introspection-2

src/networking.c

CharlesChen888 · 2022-11-08T07:04:16Z

If found one more call to lookupKeyReadWithFlags without the NOTOUCH flag, in blocked.c, function handleClientsBlockedOnKeys:
            /* Serve clients blocked on the key. */
            robj *o = lookupKeyReadWithFlags(rl->db, rl->key, LOOKUP_NONOTIFY | LOOKUP_NOSTATS);
What do you think about this one? If the key just became ready, it must have just been written and therefore just touched, so touching it again doesn't hurt? But then we could as well add the NOTOUCH flag here...

This is where I got confused. The robj *o we get here seems not just for reading. For instance, when a client is blocked when executing BLPOP, a LPOP is going to be performed to the key which becomes ready.

soloestoy · 2022-11-11T06:45:29Z

If found one more call to lookupKeyReadWithFlags without the NOTOUCH flag, in blocked.c, function handleClientsBlockedOnKeys:
            /* Serve clients blocked on the key. */
            robj *o = lookupKeyReadWithFlags(rl->db, rl->key, LOOKUP_NONOTIFY | LOOKUP_NOSTATS);
What do you think about this one? If the key just became ready, it must have just been written and therefore just touched, so touching it again doesn't hurt? But then we could as well add the NOTOUCH flag here...
This is where I got confused. The robj *o we get here seems not just for reading. For instance, when a client is blocked when executing BLPOP, a LPOP is going to be performed to the key which becomes ready.

this is introduce by #9572, do you remember why change it from lookup write to read @zuiderkwast ?

BTW, every time I see writable replica I still have to say I hate it, it messes things up.

zuiderkwast · 2022-11-15T10:31:01Z

this is introduce by #9572, do you remember why change it from lookup write to read @zuiderkwast ?

@soloestoy (regarding the lookup in lookupKeyReadWithFlags) I don't remember why but I guess we changed it to preserve the old behaviour when we changed lookupKey. Anyway, this lookup is not a normal lookup since it's not for a specific client. Later, each key will be looked up again for a specific client and command. Let's just add NOTOUCH here (or change it so it doesn't use lookupKey at all).

Btw, I don't want to talk about writeable replicas. They're not fun. 😁

soloestoy · 2022-11-16T03:55:20Z

If the key just became ready, it must have just been written and therefore just touched, so touching it again doesn't hurt? But then we could as well add the NOTOUCH flag here...

but if it's lfu, we should touch the object to increase the frequency...

Anyway, this lookup is not a normal lookup since it's not for a specific client. Later, each key will be looked up again for a specific client and command. Let's just add NOTOUCH here (or change it so it doesn't use lookupKey at all).

I don't get your point about "each key will be looked up again for a specific client and command", for example BLPOP will use the object directly without look up it again.

zuiderkwast · 2022-11-16T07:00:49Z

I don't get your point about "each key will be looked up again for a specific client and command", for example BLPOP will use the object directly without look up it again.

@soloestoy You're right... but blockingPopGenericCommand calls lookupKeyWrite before blocking the client.

BLPOP only blocks if the key doesn't exist though. If the key is created by another client using LPUSH, there is no LRU/LFU stored when our client is unblocked. I guess we do want to increment LRU/LFU for BLPOP when popping...

CharlesChen888 · 2022-11-16T14:28:58Z

So regarding the lookupKeyReadWithFlags in handleClientsBlockedOnKeys, I guess we can leave it unchanged here. If we do need to pass a NOTOUCH flag in it, we can discuss it in #11012, as it is refactoring blocking framework.

CharlesChen888 · 2022-11-16T14:51:20Z

Just had another thought: in the orginal issue #11409, we only need a stealth/no-touch mode for read operations, but should we limit it in read operations? Or is it ok to perform write operations without touching LRU/LFU stats?

If stealth/no-touch mode can also apply to write operations, it is easier to implement.

@madolson WDYT?

zuiderkwast · 2022-11-16T16:07:30Z

should we limit it in read operations? Or is it ok to perform write operations without touching LRU/LFU stats?

Is it useful or confusing?

If stealth mode is a kind of super-readonly mode that shouldn't do any changes to keys, then we should probably return an error for write commands. If it is a useful hack for administrators, then we can allow no-touch for write operations too...

src/commands/client-no-touch.json

src/networking.c

src/server.h

madolson · 2022-12-07T16:08:24Z

Discussed in the core meeting. Discussion was to decouple the stealth mode conversation, and proceed with this PR as is. Folks should take a look shortly.

madolson · 2023-01-05T00:37:39Z

@soloestoy Do you still plan on following up on this? I can help close on this if you are busy.

soloestoy · 2023-01-05T11:23:25Z

@madolson @oranagra seems this PR is finished? as the top comment said.

soloestoy · 2023-02-16T03:23:22Z

@madolson @oranagra please approve, I think it's ready to be merged.

oranagra

sorry for the long delay. forgot about it....
mostly LGTM, here are some minor comments.

src/commands/client-no-touch.json

src/module.c

tests/unit/introspection-2.tcl

madolson

Minor comments as well, looks OK to me.

src/networking.c

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

oranagra · 2023-02-20T06:07:22Z

@CharlesChen888 can you please make a redis-doc PR as well?

CharlesChen888 · 2023-02-20T06:26:00Z

@oranagra of course, I'm already writing it.

oranagra · 2023-02-21T07:30:53Z

i wonder if we should mention the change about NO-EVICT in the docs somewhere?
i.e. it should be mentioned as a behavior change in the release notes, and it doesn't belong in the command's history section (not an interface change).
it means that 7.0 and 7.2 will behave differently in that regard, but we don't usually document bug fixes in the docs..

src/networking.c

Co-authored-by: sundb <sundbcn@gmail.com>

CharlesChen888 · 2023-02-23T01:17:07Z

i wonder if we should mention the change about NO-EVICT in the docs somewhere? i.e. it should be mentioned as a behavior change in the release notes, and it doesn't belong in the command's history section (not an interface change). it means that 7.0 and 7.2 will behave differently in that regard, but we don't usually document bug fixes in the docs..

@oranagra I think we need to doc the change of RESET command anyway (for clearing no-touch flag), so just mention it alongside of no-touch?

oranagra · 2023-02-23T07:01:17Z

@CharlesChen888 my question was about documenting the history. i.e. mention that in redis 7.0 we didn't reset the no-evict and in 7.2 we do..
i guess we shouldn't do that, and we will certainly mention the change in the release notes.

CLIENT NO-TOUCH added in redis#11483, but forgot to add the since field in the JSON file. This PR adds the since field to it with a value of 7.2.0

CLIENT NO-TOUCH added in #11483, but forgot to add the since field in the JSON file. This PR adds the since field to it with a value of 7.2.0

@madolson

…/LFU of keys (redis#11483) When no-touch mode is enabled, the client will not touch LRU/LFU of the keys it accesses, except when executing command `TOUCH`. This allows inspecting or modifying the key-space without affecting their eviction. Changes: - A command `CLIENT NO-TOUCH ON|OFF` to switch on and off this mode. - A client flag `#define CLIENT_NOTOUCH (1ULL<<45)`, which can be shown with `CLIENT INFO`, by the letter "T" in the "flags" field. - Clear `NO-TOUCH` flag in `clearClientConnectionState`, which is used by `RESET` command and resetting temp clients used by modules. - Also clear `NO-EVICT` flag in `clearClientConnectionState`, this might have been an oversight, spotted by @madolson. - A test using `DEBUG OBJECT` command to verify that LRU stat is not touched when no-touch mode is on. Co-authored-by: chentianjie <chentianjie@alibaba-inc.com> Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com> Co-authored-by: sundb <sundbcn@gmail.com>

CLIENT NO-TOUCH added in redis#11483, but forgot to add the since field in the JSON file. This PR adds the since field to it with a value of 7.2.0

no-evict added in redis/redis#8687 no-touch added in redis/redis#11483 Co-authored-by: Binbin <binloveplau1314@qq.com>

zeekling reviewed Nov 7, 2022

View reviewed changes

src/expire.c Outdated Show resolved Hide resolved

zuiderkwast reviewed Nov 7, 2022

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

zuiderkwast reviewed Nov 7, 2022

View reviewed changes

madolson reviewed Nov 7, 2022

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

CharlesChen888 changed the title ~~Add client stealth mode for not changing LRU/LFU stats when performing read operations.~~ Add client no-touch mode for not changing LRU/LFU stats when looking up keys. Nov 17, 2022

zuiderkwast reviewed Nov 17, 2022

View reviewed changes

src/commands/client-no-touch.json Outdated Show resolved Hide resolved

zuiderkwast reviewed Nov 17, 2022

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

zuiderkwast reviewed Nov 17, 2022

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

CharlesChen888 requested review from madolson and oranagra and removed request for madolson and oranagra November 18, 2022 08:15

soloestoy approved these changes Jan 5, 2023

View reviewed changes

oranagra reviewed Feb 16, 2023

View reviewed changes

src/commands/client-no-touch.json Outdated Show resolved Hide resolved

src/commands/client-no-touch.json Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

tests/unit/introspection-2.tcl Outdated Show resolved Hide resolved

Add client no-touch mode.

8154e1d

Fail when can't get LRU with debug object.

082ee75

madolson reviewed Feb 18, 2023

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

CharlesChen888 and others added 3 commits February 18, 2023 12:02

Change info string of no-touch mode.

eed2fdf

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

Clear no-touch flag in clearClientConnectionState.

7bf3a5d

Clear no-evict flag in clearClientConnectionState.

387eb3d

oranagra approved these changes Feb 20, 2023

View reviewed changes

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 20, 2023

oranagra added the state:needs-doc-pr requires a PR to redis-doc repository label Feb 20, 2023

CharlesChen888 mentioned this pull request Feb 21, 2023

Add CLIENT NO-TOUCH. Update RESET command. redis/redis-doc#2332

Merged

madolson approved these changes Feb 21, 2023

View reviewed changes

sundb reviewed Feb 21, 2023

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

Remove redundant arity check of client no-touch.

201812c

Co-authored-by: sundb <sundbcn@gmail.com>

oranagra changed the title ~~Add client no-touch mode for not changing LRU/LFU stats when looking up keys.~~ Add CLIENT NO-TOUCH to allow clients to inspect or modify keys without affecting LRU/LFU Feb 23, 2023

oranagra changed the title ~~Add CLIENT NO-TOUCH to allow clients to inspect or modify keys without affecting LRU/LFU~~ Add CLIENT NO-TOUCH for clients to run commands without affecting LRU/LFU of keys Feb 23, 2023

oranagra merged commit 897c3d5 into redis:unstable Feb 23, 2023

enjoy-binbin mentioned this pull request Feb 23, 2023

Add missing since filed for new CLIENT NO-TOUCH command #11829

Merged

oranagra pushed a commit that referenced this pull request Feb 23, 2023

Add missing since filed for new CLIENT NO-TOUCH command (#11829)

61acf51

CLIENT NO-TOUCH added in #11483, but forgot to add the since field in the JSON file. This PR adds the since field to it with a value of 7.2.0

alexprengere mentioned this pull request Mar 24, 2023

Add support for CLIENT NO-TOUCH redis/redis-py#2662

Closed

enjoy-binbin mentioned this pull request Oct 23, 2023

update client list fields: no-evict and no-touch redis/redis-doc#2572

Merged

oranagra pushed a commit to redis/redis-doc that referenced this pull request Oct 24, 2023

update client list fields: no-evict and no-touch (#2572)

f5514bf

no-evict added in redis/redis#8687 no-touch added in redis/redis#11483 Co-authored-by: Binbin <binloveplau1314@qq.com>

Add CLIENT NO-TOUCH for clients to run commands without affecting LRU/LFU of keys #11483

Add CLIENT NO-TOUCH for clients to run commands without affecting LRU/LFU of keys #11483

Uh oh!

Conversation

CharlesChen888 commented Nov 7, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CharlesChen888 commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soloestoy commented Nov 11, 2022

Uh oh!

zuiderkwast commented Nov 15, 2022

Uh oh!

soloestoy commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Nov 16, 2022

Uh oh!

CharlesChen888 commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharlesChen888 commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Nov 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented Dec 7, 2022

Uh oh!

madolson commented Jan 5, 2023

Uh oh!

soloestoy commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soloestoy commented Feb 16, 2023

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oranagra commented Feb 20, 2023

Uh oh!

CharlesChen888 commented Feb 20, 2023

Uh oh!

oranagra commented Feb 21, 2023

Uh oh!

Uh oh!

CharlesChen888 commented Feb 23, 2023

Uh oh!

oranagra commented Feb 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CharlesChen888 commented Nov 7, 2022 •

edited by oranagra

Loading

CharlesChen888 commented Nov 8, 2022 •

edited

Loading

soloestoy commented Nov 16, 2022 •

edited

Loading

CharlesChen888 commented Nov 16, 2022 •

edited

Loading

CharlesChen888 commented Nov 16, 2022 •

edited

Loading

soloestoy commented Jan 5, 2023 •

edited

Loading