search

StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.89k stars 1.51k forks source link

Set LibraryVersion in ConfigurationOptions #2533

Open shacharPash opened 1 year ago

shacharPash commented 1 year ago

I saw that now we can set the LibraryName in the ConfigurationOptions, what about LibraryVersion? can we add LibraryVersion as well like here?

NickCraver commented 1 year ago

What is the use case for changing it? The library name being modifiable to add more per-wrapping-application info was the intent with making it changeable, but the version is what we compile here and has a lot of value on the backend.

shacharPash commented 1 year ago

@NickCraver In NRedisStack I want to send the lib-name and lib-ver automatically (unless otherwise specified) when the client creates a connection, and I can easily do this with ConfigurationOptions if I can also set the LibraryVersion.

NickCraver commented 1 year ago

@shacharPash I think that makes sense, it's not how I think about it as seeing which versions are connecting and a server making decisions about that. For example, if I were seeing if all my clients were capable of X, this hides that ability since the library overriding these values could be anything, and using any version of SE.Redis underneath, making assessing what versions people are actually connecting with impossible.

Thoughts on the downsides there?

shacharPash commented 1 year ago

Thanks for your consideration, In my case I need to be able to send the lib-name&version of NRedisStack. I don't see a particular downside here, for example, I thought of sending something like this:

LibraryName: NRedisStack (SE.Redis)
LibraryVersion: 0.9.1 (2.68.23423)

then the name and version of SE.Redis and NRedisStack will be written.

mgravell commented 1 year ago

I'm broadly OK with enabling this; if we don't folks will just bypass via Execute, so... maybe we make it easy to do well, perhaps with placeholder tokens for the default values?

shacharPash commented 1 year ago

Hey @mgravell @NickCraver , Do you want me to send a PR or are you working on it?

shacharPash commented 1 year ago

Hi, just updating that in the SETINFO command it is not possible to use ( and ), therefore if we want to write both SE.Redis and the library that uses it, we will have to write it in a different way from this:

LibraryName: NRedisStack (SE.Redis)
LibraryVersion: 0.9.1 (2.68.23423)
NickCraver commented 1 year ago

@shacharPash Sorry didn't see your latest there - I wanted to get RESP3 in before doing this as that whole area conflicts, but it's in now - was taking a look this morning. My plan is to separate these 2, and just outright allow overwriting LibraryVersion, and adding tokens to both name and version in a follow-up PR. Taking a peek at it now.

NickCraver commented 1 year ago

Working this in #2547