StackExchange.Redis icon indicating copy to clipboard operation
StackExchange.Redis copied to clipboard

Set LibraryVersion in ConfigurationOptions

Open shacharPash opened this issue 1 year ago • 9 comments

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

shacharPash avatar Aug 29 '23 14:08 shacharPash

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.

NickCraver avatar Aug 29 '23 16:08 NickCraver

@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.

shacharPash avatar Aug 30 '23 07:08 shacharPash

@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?

NickCraver avatar Sep 01 '23 12:09 NickCraver

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.

shacharPash avatar Sep 03 '23 13:09 shacharPash

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?

mgravell avatar Sep 03 '23 13:09 mgravell

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

shacharPash avatar Sep 07 '23 13:09 shacharPash

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)

shacharPash avatar Sep 10 '23 07:09 shacharPash

@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 avatar Sep 10 '23 12:09 NickCraver

Working this in #2547

NickCraver avatar Sep 10 '23 13:09 NickCraver