valkey icon indicating copy to clipboard operation
valkey copied to clipboard

The reply from HELLO

Open zuiderkwast opened this issue 10 months ago • 20 comments

> hello 3
1# "server" => "redis"
2# "version" => "7.2.4"
(...)

I think we should change it anyway, immediately in our first compatibility version. It is very low risk that any client depends on this value, since it's static.

For a better decision, what do other forks and redis-compatible services (AWS, Alibaba, ...) return for this field?

zuiderkwast avatar Mar 28 '24 11:03 zuiderkwast

For this kind of similar commands, I prefer to keep current field and add more fields to compatible with Redis 7.2.4.

hwware avatar Mar 28 '24 14:03 hwware

Problem: When we change it, we make it incompatible with our own previous version.

In some cases, we can have both names overlapping. That includes filenames (one filename is a symlink to the other) and INFO fields. For those, we can introduce our new fields. Then at a later major version, we can remove the old names. It's not possible with HELLO server name.

zuiderkwast avatar Mar 28 '24 14:03 zuiderkwast

Problem: When we change it, we make it incompatible with our own previous version.

In some cases, we can have both names overlapping. That includes filenames (one filename is a symlink to the other) and INFO fields. For those, we can introduce our new fields. Then at a later major version, we can remove the old names. It's not possible with HELLO server name.

yes, agree. Later major version, remove the old name.

hwware avatar Mar 28 '24 14:03 hwware

Good that we agree about the other issues.

Note: For this issue (HELLO) it is not possible to have an overlapping period of two names.

zuiderkwast avatar Mar 28 '24 14:03 zuiderkwast

My concern is that redis might update their clients to look for this field, and error out if they see it. I suppose that is a risk for any user facing change we make. Maybe we should add a config for the compatibility layer?

madolson avatar Mar 28 '24 15:03 madolson

Good point @madolson. That's why I'm asking what does ElastiCache return for this field? "redis"?

zuiderkwast avatar Mar 28 '24 15:03 zuiderkwast

We just return redis today. I think all the major forks do too.

madolson avatar Mar 28 '24 15:03 madolson

OK, very good plan. Let's keep redis for now. Soon enough, let's add a compatibility switch with default ON. (Next minor milestone)

zuiderkwast avatar Mar 28 '24 16:03 zuiderkwast

I can confirm that there are client libraries/frameworks that expect to see the redis version.

romange avatar Mar 28 '24 17:03 romange

One of such frameworks is sidekiq.

romange avatar Mar 28 '24 17:03 romange

My concern is that redis might update their clients to look for this field, and error out if they see it. I suppose that is a risk for any user facing change we make. Maybe we should add a config for the compatibility layer?

Would it be possible to make the value user configurable in valkey.conf?

compat-hello-server "valkey" yields

> hello 3
1# "server" => "valkey"
...

compat-hello-server "redis" yields

> hello 3
1# "server" => "redis"
...

compat-hello-server "foobar" yields

> hello 3
1# "server" => "foobar"
...

In later versions a user can make it say whatever they need for compatibility. Same could be done for version numbers, but that's more risky.

stockholmux avatar Mar 28 '24 19:03 stockholmux

Not sure why we need so much tuning, I think just a single flag which indicates what we are doing should be sufficient? I'm thinking just a flag like. redis-compatibility-type [strict, loose, none]. Which does:

  1. strict: We look like Redis, nothing else.
  2. loose: We look like Redis, but show some extra fields.
  3. None: We show zero redis anywhere.

madolson avatar Mar 28 '24 19:03 madolson

So, say there is a change in client libs and a new version of redis which makes whatever strict implementation no longer work. User configurable bits means something like this could just be tweak to your own config file, not valkey revv'ing a release.

I can also think of scenarios where a client lib could look for one thing but maybe a user could sniff somewhere else to know it's running valkey, having tuning here would help.

stockholmux avatar Mar 28 '24 19:03 stockholmux

So, say there is a change in client libs and a new version of redis which makes whatever strict implementation no longer work. User configurable bits means something like this could just be tweak to your own config file, not valkey revv'ing a release.

Let's not worry about future hypothetical. For the next version, top level configuration should be more than enough. It is simple and less clutter for users in config file.

I can also think of scenarios where a client lib could look for one thing but maybe a user could sniff somewhere else to know it's running valkey, having tuning here would help.

It is not a problem if a user can see valkey under the hood. We are not trying to masquerade as Redis, only providing support to run existing software without much heavy lifting for users.

parthpatel avatar Mar 28 '24 21:03 parthpatel

True, this compatibility layer can't be a stealth mode. I don't think any client would try some command that returns an error just to check that error message contains "Redis". That would mean we can never clean up some error messages.

zuiderkwast avatar Mar 28 '24 22:03 zuiderkwast

Maybe like browser User Agent: valkey, like redis.

(inspired by like Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36 from https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent)

bradh avatar Mar 31 '24 02:03 bradh

So, say there is a change in client libs and a new version of redis which makes whatever strict implementation no longer work. User configurable bits means something like this could just be tweak to your own config file, not valkey revv'ing a release.

This would also be a breaking change for clients. I agree about not worrying too much about that, since we can only stay truly compatible with 7.2.

madolson avatar Mar 31 '24 04:03 madolson

Keeping "server" => "redis" forever implies that we're also keeping "version" => "7.2.4" forever, right?

We can add fields though? That can't break anyone, right? Let's consider it. It's useful to get this from HELLO so the client can infer what features are available, such as client-side tracking or future features like async blocking commands, even if those are opt-in.

zuiderkwast avatar Apr 05 '24 07:04 zuiderkwast

My preference is still let's not touch hello, and consider a larger change for 8.0. I think we can consider changing the server to valkey in the future.

madolson avatar Apr 05 '24 16:04 madolson

Yes, I'm only talking about 8.0 here. No change for 7.2.

No need to make a decision about it now, but it can be good to keep in mind if we want to align terminology with INFO fields for example.

zuiderkwast avatar Apr 05 '24 16:04 zuiderkwast

For Valkey 8, I suggest we change it to

"server" => "valkey"
"version" => "8.0.0"

In addition to this, we add a redis-compatibility config #274, disabled by default but users can enable it if they have problems. When enabled, it will instead pretend to be Redis 7.2.4:

"server" => "redis"
"version" => "7.2.4"

zuiderkwast avatar Apr 10 '24 09:04 zuiderkwast