launchdarkly / ld-relay

LaunchDarkly Relay Proxy
Other
112 stars 80 forks source link

Relay can't handle Moved error from Clustered Redis. #46

Closed matthalbersma closed 6 years ago

matthalbersma commented 6 years ago

Using clustered Redis on AWS. I get the following error.

[LaunchDarkly Relay (SdkKey ending with 33f4b)] 2018/10/22 16:35:00 Starting LaunchDarkly streaming connection [LaunchDarkly Relay (SdkKey ending with 33f4b)] 2018/10/22 16:35:00 Connecting to LaunchDarkly stream using URL: https://stream.launchdarkly.com/all [LaunchDarkly Relay (SdkKey ending with 33f4b)] 2018/10/22 16:35:00 Error initializing store: MOVED 5868 10.100.150.235:6379 [LaunchDarkly Relay (SdkKey ending with 33f4b)] 2018/10/22 16:35:10 Timeout exceeded when initializing LaunchDarkly client.

I would expect that the Launch Darkly client could handle the Redis Moved error and redirect to the appropriate node. Is no one else using clustered Redis? If not is there a recommended Redis implementation?

eli-darkly commented 6 years ago

Although the go-redis library that the LaunchDarkly Go SDK uses does support clustered Redis, the clustered Redis client API is not interchangeable with the regular Redis client API and currently our SDK only uses the latter. Since the relay is built around the SDK, it has the same limitation.

We've had a few requests for clustered Redis support, but I'm not aware of anyone currently using it with LaunchDarkly. If they are, then it would be via their own fork of the code.

matthalbersma commented 6 years ago

Thanks for the reply. I'll close the issue.

luismojena commented 4 years ago

Has the support for clustered Redis been added?

eli-darkly commented 4 years ago

@luismojena No. It's still something we would like to do, but there were three issues: 1. it is harder to write an adapter using Redigo that can do both clustered and non-clustered mode, 2. if we switched from ~go-redis to Redigo~ Redigo to go-redis, which makes that easier, we would lose compatibility with Go versions that we still support, and 3. there are some design issues we still have to resolve (the way the SDK uses keys and hashes in Redis does not fit well with the way clustering is supposed to distribute keys across nodes). Issues 1 and 2 should not be such a problem once we release version 5.0 of the Go SDK, but issue 3 is still holding up our work on this.

luismojena commented 4 years ago

@eli-darkly Thanks a lot for the response.

eli-darkly commented 4 years ago

@luismojena Here's a little more detail if you're interested.

Much of the underlying functionality in Relay, like getting flags from LaunchDarkly, storing flags, and evaluating flags, comes from our Go SDK. Previously, the Go SDK contained all of the database integrations we provide (Redis, Consul, and DynamoDB) in subpackages within the same repository. The Redis one was specifically the Redigo implementation as I mentioned.

In the 5.0 release, we're making those database integrations into separate projects, removing them from the SDK repository. That gives us more flexibility to update the integrations independently, and release new ones that don't have to have the exact same compatibility requirements as the SDK— for instance, the go-redis client can only be used as a Go module, but for the SDK itself (in this release) we are still maintaining compatibility with non-module-based applications. So, we will be adding a go-redis-based integration, based on a user-contributed PR. And then we will have the option of making Relay use go-redis, which makes clustered-Redis compatibility easier to achieve.

For the third issue that I mentioned, we will probably stick with the same schema of keys and hash keys that we've been using (to avoid potential confusion if the same Redis instance is being accessed by other SDKs); it's just that it might be possible to use clustered Redis more efficiently if we changed this, but for various reasons, mainly about the behavior of transactions, that might not be feasible. So that's less likely to hold up the work, but we do need to think it through.

Foda commented 4 years ago

Just wanted to bump this, but we (Bitbucket) are currently blocked on not being able to use the relay because of a lack of clustering support. Due to the way our app works, we need to utilize Envoy to do connection pooling to the Redis node, and Envoy doesn't support non-clustered Redis...