search

launchdarkly / dotnet-server-sdk-redis

Redis integration for the LaunchDarkly SDK for Server-side .NET
Other
2 stars 3 forks source link

Upgrade to StackExchange.Redis 2 #10

Closed peter-majeed closed 3 years ago

peter-majeed commented 3 years ago

StackExchange.Redis is on version 2, but these libraries only reference v. 1.2.6. StackExchange.Redis 1.2.6 isn't strongly signed, and StackExchange.Redis.StrongName has been deprecated. Are there plans to update this library to allow for compatibility with the latest StackExchange.Redis packages? Happy to put in a PR if so.

eli-darkly commented 3 years ago

Yes, we are planning to update to StackExchange.Redis 2.x in the next major version of this library, which should be fairly soon. The plan was for this release to correspond with the release of .NET SDK 6.0, which we're hoping people will adopt readily since it's a major improvement in several ways. However, if there's demand for a version of the LD Redis integration that works with StackExchange.Redis 2.x and .NET SDK 5.x, maybe we should consider releasing a 2.0 version of this package that only has the StackExchange.Redis change, and then having 3.0 be the one that requires .NET SDK 6.x.

peter-majeed commented 3 years ago

Aha, thanks Eli. Yeah, I didn't see the upgrade to the 2.0 versions of StackExchange.Redis in the nuget pre-release versions or in the open PRs, so figured I'd ask. I think there's probably value in doing the upgrade to the later version of the Redis library. I just tried upgrading locally against my fork. There weren't a lot of changes outside of the fact that RedisConfig.ResponseTimeout is now deprecated, and it'd at least unblock me. 🙂

eli-darkly commented 3 years ago

Yeah, I think we should go ahead and do this ASAP in a 2.0 version, and bump our upcoming bigger changes to 3.0.

We normally try to keep our transitive dependencies at the lowest possible compatible version, on the assumption that it's easier for application developers to upgrade it by adding their own dependency, whereas if we used a version that's higher than what they want it's harder to downgrade. So, based on their changelog, I think that would be 2.0.513 - they seem to have made some potentially incompatible dependency changes in that release, but not afterward. And we'll need to drop compatibility with .NET Standard 1.6 and .NET Framework 4.5.

eli-darkly commented 3 years ago

@peter-majeed Hmm. I see that ResponseTimeout is not only deprecated, but non-functional. It looks like we should be using SyncTimeout instead, is that right?

eli-darkly commented 3 years ago

(As you noticed, we had not gotten around to this work yet on the next-major-version branch of this repo, so I'm only catching up on these issues now...)

peter-majeed commented 3 years ago

No worries. From my surface reading of the underlying issue, it looks like ResponseTimeout is no longer necessary @ https://github.com/StackExchange/StackExchange.Redis/pull/1042. The maintainers didn't seem to give much explanation why. (I didn't dig deeper myself.)

From another issue conversation, it appears that ResponseTimeout was intended to address dead connections, but SyncTimeout appears to address specific operations @ https://github.com/StackExchange/StackExchange.Redis/issues/235, so I think they wouldn't necessarily be a drop-ins for each other.

peter-majeed commented 3 years ago

Just put in the PR @ https://github.com/launchdarkly/dotnet-server-sdk-redis/pull/11, happy to let this sit as needed

eli-darkly commented 3 years ago

We've released LaunchDarkly.ServerSdk.Redis 2.0.0, which updates the library to use StackExchange.Redis 2.x.

Note that this includes an increase in the minimum LaunchDarkly.ServerSdk version to 5.14.0, because we've made some updates to the configuration API in the SDK which we need to support in the latest releases of this package. So to use this you will need to update your LaunchDarkly.ServerSdk dependency, which will cause some deprecation warnings to appear in places where you've used the older APIs.

Please let us know when you've had a chance to try this out.

peter-majeed commented 3 years ago

@eli-darkly this worked for us. Thanks for taking this on!