search

valkey-io / valkey

A flexible distributed key-value datastore that is optimized for caching and other realtime workloads.
https://valkey.io
Other
17.52k stars 661 forks source link

[BUG] Wrong version number #392

Open furai opened 7 months ago

furai commented 7 months ago

Describe the bug

Valkey reports wrong version number.

To reproduce

  1. Compile valkey from 7.2.5 tag.
  2. Do valkey-cli --version (or any other compiled library).
  3. Result is 7.2.4

Expected behavior

It should return 7.2.5.

Additional information

I've noticed this while working on my AUR package. Maybe I'm compiling something wrong. Here's link to the package for reference: https://aur.archlinux.org/packages/valkey and https://aur.archlinux.org/packages/valkey-git

Shivshankar-Reddy commented 7 months ago

Thanks for reporting the issue! Looks like valkey-cli's cliVersion api is not updated with VALKEY_VERSION. image

Shivshankar-Reddy commented 7 months ago

@zuiderkwast @madolson 7.2.5 code base looks different, for instance cliVersion definition available in valkey-cli.c and in unstable its available in cli_common.c file. Because this in-consitency valkey changes not appled otherwise should be fixed in https://github.com/valkey-io/valkey/pull/232

madolson commented 7 months ago

Yeah, the code changed quite a bit between 7.2 and unstable, and we were trying to be very judicious about the backport. I don't think we thought about this one. It probably makes sense to update this to be the server_version instead of the redis version though.

furai commented 7 months ago

For me personally as a new user not related to the project it makes more sense to have 7.2.5 version everywhere when I compile valkey. I'm thinking from a perspective where no prior knowledge of redis is assumed, like totally new person to the topic. For them it would be confusing (and for me in this instance as well) that when they check what version of software they're using and they're getting different number than expected.

Same goes for the default decision to have redis-cli and what not symlinked. These in my opinion shouldn't be symlinked by default. On top of that, because of that symlink, both redis and valkey can't coexist on the system cause the install locations conflict.

hwware commented 7 months ago

@furai Since you a new user for our Valkey, I just have one question. Do you feel confused as the following highlight part in json file?

image

I am not sure, Welcome feeback. Thanks

furai commented 7 months ago

You mean that it's 6.0.0 when the project was still redis? That's something for documentation, right? I mean, it's kept for historical reasons, you can go in this repo and find 6.0.0 version. That's versioning of the software and it makes perfect sense. The naming redis/valkey is just a sprinkle on top and I wouldn't touch that. Definitely I wouldn't prepend 6.0.0 with redis word. That would be even more confusing.

I'd for sure wouldn't want to mix versioning and "branding". For me it doesn't matter that this previously was named redis. All I care about is knowing that it was introduced at some point in the past, certain version.

As to my original issue - we have released version 7.2.5 and it wasn't reflected in --version for certain binaries. I don't think it should state that it was redis-7.2.4 as this would be even more confusing. I get that feature-wise it was on par with 7.2.4 version of redis but that's totally irrelevant for someone like me.

That's just my personal feeling about the whole issue.

zuiderkwast commented 7 months ago

@furai You're right, this is confusing. I'm not sure what we need to do with exactly this bug. Maybe we can fix it and release it as 7.2.6.

So why is it like this? Our 7.2.x is a "redis compatibility release". We were trying to keep everything "redis 7.2.4 compatible" and pretend that we are Redis. It makes no sense for you but it can be important for other users. Some commands return error message like "Redis is busy" and it was important to keep things like that, because some clients and tools are hard-coded to check for exactly these strings.

In 8.0 we will have a much better situation. We will have a extended-redis-compatiblity config, default off. When it's on, valkey will pretend to be Redis 7.2.4 and when it's off (by default) it will be Valkey 8.0.0.

Same goes for the default decision to have redis-cli and what not symlinked. These in my opinion shouldn't be symlinked by default. On top of that, because of that symlink, both redis and valkey can't coexist on the system cause the install locations conflict.

The reason is that some tools and user scripts assume that there are commands like redis-cli, so to be redis-compatible we should provide them. It's possible to disable these when installing.

You have the same problem when installing two different versions of valkey or two different versions of redis. It's possible to install them in different directories. Another possibility is to add a suffix to the binaries' names, make PROG_SUFFIX="-foo" will compile binaries with names like valkey-cli-foo.

madolson commented 6 months ago

So I guess we are saying that for 7.2, it will stay compatible with Redis, then when release Valkey 8.0 it will reflect the valkey version?

zuiderkwast commented 6 months ago

Yes, in Valkey 8 we will distinguish between the Valkey version and the Redis compat version and return one or the other in various places.

But in Valkey 7.2.x we don't distinguish between valkey version and redis-compat-version.

If we make a patch for this in the 7.2 branch, it probably doesn't harm to return 7.2.5 or 7.2.6 here. Although there is not yet any such Redis OSS version, if there will be, it won't have any new features or breaking changes.