search

valkey-io / valkey

A new project to resume development on the formerly open-source Redis project. We're calling it Valkey, since it's a twist on the key-value datastore.
https://valkey.io
Other
14.65k stars 520 forks source link

Client-side caching "__redis__:invalidate" #280

Open zuiderkwast opened 2 months ago

zuiderkwast commented 2 months ago

Invalidation messages for client-side caching (see CLIENT TRACKING) are sent to the special channel __redis__:invalidate.

Simply changing it would make Valkey incompatible with Redis OSS 7.2, so we don't want to do it like that. We want to change this channel name in a backward-compatible way:

If the client is subscribed to __redis__:invalidate, we send the invalidation messages to this channel and if subscribed to __valkey__:invalidate we send to that channel.

hpatro commented 2 months ago

@zuiderkwast Now might be the best time to reserve some channel namespace for server side purpose.

And should we disable external publish on those reserved channels?

zuiderkwast commented 2 months ago

We can add a channel alias now (in the 7.2.4-rc2) if we're really fast. I don't know if it's necessary though. WDYT?

Maybe we should call it __valkey__:invalidate because it's the least confusing name for users switching from redis... Or do you have a different idea?

Forbid push to reserved channels is a breaking change, so that's for 8.0, ok?

Can we forbid publish to any channel starting with double underscore?

hwware commented 2 months ago

Could we have a way just add an alias for this and make the redis:invalidate as deprecated, then after releasing several major release, we could remove it, Thanks

hpatro commented 2 months ago

We can add a channel alias now (in the 7.2.4-rc2) if we're really fast. I don't know if it's necessary though. WDYT?

Maybe we should call it __valkey__:invalidate because it's the least confusing name for users switching from redis... Or do you have a different idea?

I think we can go for __valkey__:. I believe all of these will be under one compatibility config flag, right?

Forbid push to reserved channels is a breaking change, so that's for 8.0, ok?

Can we forbid publish to any channel starting with double underscore?

__ might be widely used and impact lot of clients, if we stick with __valkey__ for server side purpose, we should forbid publish on that.

madolson commented 2 months ago

We discussed reserving a namespace in the past, and the decision was that we shouldn't reserve it because that prevents clients from also sending along that channel. One use case that was mentioned is it allows a single way to do refresh ahead caching. (you can either let the key get deleted, which generates a message, or send a message to it).

hpatro commented 2 months ago

Albeit a breaking change, by reserving channel(s) for server side, we gain few things:

  1. The schema of the message remains the same, doesn't create any problem around parsing.
  2. Introduce new channel in the future for different purpose(s) like slot migration. update, cluster topology refresh, etc and it doesn't conflict with clients channel namespace.

We discussed reserving a namespace in the past, and the decision was that we shouldn't reserve it because that prevents clients from also sending along that channel. One use case that was mentioned is it allows a single way to do refresh ahead caching. (you can either let the key get deleted, which generates a message, or send a message to it).

One recommendation could be clients perform a psubscribe on __*__:invalidate which would handle __redis__ , __valkey__, __myownchannel__

zuiderkwast commented 2 months ago

Can we move "reserving channel(s)" to a separate issue?

For the invalidate messages, I've investigated a little. If one client does CLIENT TRACKING ON REDIRECT 8 and client 8 is in subscribed to any channel (let's say "ch1"), Valkey will send the invalidation message to client 8 anyway with channel name __redis__:invalidate, even though the client didn't subscribe to that channel.

This behaviour makes sending to both channels a little more complicated.

My suggestion is that we check if the client is subscribed to __valkey__:invalidate and if yes, we send it on that channel. Otherwise, we send it on __redis__:invalidate to keep backwards compatibility. (This is for RESP2 only btw.)

PingXie commented 2 months ago

This behaviour makes sending to both channels a little more complicated.

Can you elaborate a bit about the problem of double publishing in this case? I would assume the client will no-op on the messages if it doesn't recognize the channel over which the messages were sent, just like the non-redirection case?

zuiderkwast commented 2 months ago

@PingXie how do you know all clients would do that?

Besides, i wouldn't want to send two messages every time. Invalidations can be many and can eat a lot of bandwidth.

just like the non-redirection case?

Not sure what you mean by this.

PingXie commented 2 months ago

@PingXie how do you know all clients would do that?

I don't know.

I was just comparing the non-redirection case (where the client is getting unsolicited messages) vs the normal case (where the client actively looks for messages). I thought you are saying the double publishing idea works for the normal case but not the unsolicited case? My understanding is IF it works for the normal case, it should work for the unsolicited case as well. But I agree that is a big IF that I don't have a high confidence on either atm.

Besides, i wouldn't want to send two messages every time. Invalidations can be many and can eat a lot of bandwidth.

That's my concern too.

Not sure what you mean by this.

I meant the unsolicited case of CLIENT TRACKING ON REDIRECT 8

zuiderkwast commented 2 months ago

(If I'm not mistaken) Non-redirection works for RESP3 only and the client gets push messages. They're not unsolicited. Why would they be?

zuiderkwast commented 2 months ago

I meant the unsolicited case of CLIENT TRACKING ON REDIRECT 8

Where client 8 is in pubsub mode but didn't subscribe to that particular channel... It's a corner case for sure, but it'd ve a breaking change to change it, wouldn't it? (We could consider it a bugfix maybe.)

PingXie commented 2 months ago

(If I'm not mistaken) Non-redirection works for RESP3 only and the client gets push messages. They're not unsolicited. Why would they be?

I think this code pointer is what you are referring to. My understanding is that a RESP 3 client does not need to be in the pub/sub mode in order for the server to deliver the cache invalidation message but for a RESP 2 client, it needs to be in the pub/sub mode though not required to subscribe to the invalidation channel. Both are "unsolicited" IMO but yes there is more "surprise" in the RESP3 case.

Nonetheless, this is probably a digression from your original topic already. I was just trying to get the background picture.

Where client 8 is in pubsub mode but didn't subscribe to that particular channel... It's a corner case for sure, but it'd ve a breaking change to change it, wouldn't it? (We could consider it a bugfix maybe.)

Yes it would've been a breaking change even if we double-publish, since it is hard to speculate the client behavior.

I wonder if we should just consider "channel names" part of the extended wire protocol and level them as-is. The RESP protocol is incomplete IMO as it covers the syntax parts only but semantics matter too, as seen in this case.

zuiderkwast commented 3 weeks ago

If a client does PSUBSCRIBE with * or __*__:invalidate or any other pattern, do we then want to send two messages, one on the __redis__:invalidate channel and one on __valkey__:invalidate? I don't think so. It consumes extra bandwidth and it's not really better than just using __redis__:invalidate only.

What can we expect a client to do with an unknown message? I think it depends on the client, but I'd guess one of the following:

  1. Ignore it
  2. Log it
  3. Assert

Given this is a fallback for RESP2 only, and clients are encouraged to use RESP3 (or at least I think we should encourage them to do so), my vote is to just leave this as __redis__:invalidate.