@soloestoy this informmation is very useful, I was trying to do the impementation also.. However I noticed there is no such APIs for supporting like a CLIENT INFO command, which I would think like showing current info of the single client in CLIENT LIST command, we can put the tracking information there and showing some extras that we may interested with. How do you think?

One of the reason I can think of splitting the information only for that client from CLIENT LIST is we may put an ACL rules on this, to isolate user can only see the current client status

I agree with @hwware. I like the idea of adding this to client list as tracking flags, tracking prefixes, and redirect client. I'm not convinced about the utility of client info though, that could come later though.

ping @oranagra , PR updated.

+1 on at least adding this check to some of the tests.

@soloestoy can you please add some basic coverage test and respond to Madelyn's comments?
and maybe also make a PR for redis-doc.

ok @oranagra I will update it soom

@oranagra @madolson PR updated, please check.

Hi all, I modify the PR, show the original info based on tracking command option, add two section opt-mode and caching, just tell users what they did.

@redis/core-team please review & approve.

@soloestoy while documenting this I had some late reservations, specifically:

• Why use -1 if there's no redirect? I think (nil) makes more sense here
• Why is the "opt-mode" section needed? Isn't it covered in "flags"?
• WRT "caching"'s value - the values "yes"/"no" make sense given the context of CLIENT CACHING, but these values aren't really used in any replies by Redis (except perhaps some config directives). I think it should a 1/0 reply instead.

The best option I could come up with, was to collapse the last two states to this:

        /* Opt-in caching status */
        addReplyBulkCString(c,"next-command-caching-override");
        if (c->flags & CLIENT_TRACKING_OPTIN & CLIENT_TRACKING_CACHING) {
            addReplyBulkCString(c,"opt-in");
        } else if (c->flags & CLIENT_TRACKING_OPTOUT & CLIENT_TRACKING_CACHING) {
            addReplyBulkCString(c,"opt-out");
        } else {
            addReplyBulkCString(c,"none");
        }

This is part of the opt-in caching feature to reflect the current state of the next command. The intention is for this to give a user visible value for the CLIENT_TRACKING_CACHING flag.

Either that, or we just add it to the flags like @hwware mentioned, and users can figure it out.

@redis/core-team thoughts?

@madolson your proposal seems somewhat in line with what Zhao implemented in the past:

        /* Caching */
        addReplyBulkCString(c,"next-key-cached");
        if (c->flags & CLIENT_TRACKING_OPTIN) {
            addReplyBulkCString(c,c->flags & CLIENT_TRACKING_CACHING ? "yes" : "no");
        } else if (c->flags & CLIENT_TRACKING_OPTOUT) {
            addReplyBulkCString(c,c->flags & CLIENT_TRACKING_CACHING ? "no" : "yes");
        } else {
            addReplyBulkCString(c,c->flags & CLIENT_TRACKING ? "yes" : "no");
        }

I suppose the fact it was later split in this PR into two fields (which are a reflection of the flags already present) was a mistake, and the purpose you state (to give the user something easy to read) is right.

as far as i can tell, for OPT_IN both implementations will give similar result: "yes"=>"opt-in", "no"=>"none".
but for OPT_OUT it'll give "no"=>"opt-out", and , "yes"=>"none". i suppose that's desirable, right?
also, the old implementation could have give "yes" in case CLIENT_TRACKING is enabled and both OPTIN and OPTOUT are off, and your's gives "none" in that case.
i don't know CSC well enough, what's that state?

bottom line, this approach seems ok to me, i'm just trying to put everything on the table to make sure we're not overlooking something.

Yeah, it's very similar to what was originally implemented, with a couple of key differences:

1. As you said regular client tracking is removed, since I think that just adds to the confusion.
2. It's the next command, not the next key, just semantics.
3. It ignores all the overlapping states, so it's just a way of representing the CLIENT_TRACKING_CACHING flag in context.

For background information on this feature, this feature sets a default behavior for tracking, that is what the opt-in and opt-out flags are for. Executing the "CLIENT CACHING YES/NO" allows you to override that default behavior for the next command, this state is stored in the CLIENT_TRACKING_CACHING flag. We could just show this flag, which was Wen's suggestion, but it can have multiple meanings, so I think adding a little more context is useful. That being said, I don't think we need a lot of more context since other flags are listed above.

ohh, now i finally understand (and being lazy and/or overloaded, only now i finally bothered to look at the redis code). sorry about that.

this CLIENT_TRACKING_CACHING flag is all wrong IMHO. (just in redis code, the CLIENT CACHING command is ok)
sometimes CLIENT CACHING YES sets that flag to true, and sometimes CLIENT CACHING NO sets that flag to true!!??.
and depending on the opt mode this flag (being set to true) can mean to cache or to avoid caching (or the flag can be completely ignored).
a bit messy 8-), but at least it's all inside redis code (not exposed to the client protocol, so not carved in stone).

so trying to put it in my own words, both of these above pasted implementations are right. the first one stats if the "override" is currently enabled, and the second one says if the next command is gonna cache or not. (sorry for repeating your words)

this really sucks. i don't particularly like either of them 8-(

i wanna suggest a 3rd one (which is kinda similar to your last suggestion, but i think sticks to the redis awkward command terminology).

1. we delete the last two fields in the current code of the PR (opt-mode and caching)
2. we add another flag to the "flags" mbulk, that can be either caching:yes, caching:no, or completely missing.
   so then flags can either look like:

• ["optin"] - opt-in with no override
• ["optin", "caching:yes"] - opt-in with override
• ["optout"] - opt-out with no override
• ["optout", "caching:no"] - opt-out with override
• [] - no opt mode
  i think that represents best meaning of the flags (avoids confusion), in the terminology that's closets to the one of the CLIENT TRACKING / CACHING commands.
  the only thing that's ugly here is that there's a flag with : in it.

if you also like it, let me know.
if not, and i have to choose between the two posted in the last two comments, (and the one in the current version of the PR), i'd choose what @madolson suggested last. i think they're quite similar actually, the only difference is that i think my suggestion is a bit more inline with the CLIENT CACHING command's terminology.

Moving it to a flag seems fine. I still think the command that enables the feature, CLIENT CACHING [YES|NO], is confusing and I would prefer to do something that makes it more readable:

["optin"] - opt-in with no override
["optin", "track-next-command"] - opt-in with override
["optout"] - opt-out with no override
["optout", "skip-next-command"] - opt-out with override

If you feel we should keep exactly redis wording, I think what you proposed is okay too.

i'd rather keep to the existing terminology (would be easier to document).
i agree that the terminology is bad, but we can't change it now, and i'd rather not add more confusion.
maybe caching-yes is better than caching:yes? i'm not sure.

My vote would be for caching-yes/caching-no, since those at least seem like flags. So this will be the code?

if (c->flags & CLIENT_TRACKING_OPTIN) {
    addReplyBulkCString(c,"optin");
    numflags++;
    if (c->flags & CLIENT_TRACKING_CACHING) {
        addReplyBulkCString(c,"caching-yes");
        numflags++;        
    }
}
if (c->flags & CLIENT_TRACKING_OPTOUT) {
    addReplyBulkCString(c,"optout");
    numflags++;
    if (c->flags & CLIENT_TRACKING_CACHING) {
        addReplyBulkCString(c,"caching-no");
        numflags++;        
    }
}

It'll replace the existing flag code for option and output.

Agreed and updated doc