Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: ConfigureEndpoints for Proxies #2383

Closed
wants to merge 10 commits into from

Conversation

usjpin
Copy link

@usjpin usjpin commented Feb 27, 2023

Hi team, this PR is a proposal to add new interface APIs ConfigureEndpoints and ConfigureEndpointsAsync to ConnectionMultiplexer.

In a distributed environment, we frequently have updates in endpoints from service discovery - due to node refreshes, expansion / shrinking of instances, reschedules from hardware issues, etc.

For Redis Clusters, this is not an issue since the library is able to rebalance slots using CLUSTER NODES. However for proxies, this command is not supported. The current solution we use is to delete and recreate a new ConnectionMultiplexer, which is not ideal due to the incurred downtime.

The new changes allow for obsolete proxy connections to be replaced, without losing availability of the ConnectionMultiplexer as a whole.

Would like to hear thoughts on this.

@usjpin usjpin marked this pull request as ready for review February 27, 2023 19:55
@NickCraver
Copy link
Collaborator

@usjpin The main issue is changing endpoints during runtime and outside of our connections and reconnections has a lot of issues, that's why it's explicitly left out of being reconfigurable or respecting the latest ConnectionOptions instance (there are a lot of race conditions when swapping the lists of servers, the endpoints, bridges, connections, and socket we have).

I'm not sure this is a case we want to support, at least in this way. The more appropriate API would be to respect ConnectionOptions's endpoint list in a reconfigure instead of having a specific API for it...but to do that, we have to solve all the edge cases and I'm not sure it's worth it. I think I understand the use case, but we'd want to approach this in a more generic way and not as a one-off API :)

@usjpin
Copy link
Author

usjpin commented Mar 1, 2023

Hi @NickCraver , thanks for the response. I'm wondering if you could give me some example of the issues? I modeled my logic after what happens during Redis Cluster rebalancing. The old connections are marked unselectable and ReconfigureAsync()on top of the newly added ones.

I understand that we might not want to support this as an API and instead in ConfigurationOptions like you said (which will probably uncover issues for the other modes of use). But I'm wondering what kind of runtime issues this may run into in the case it is used as an API just for proxies? Maybe I'm misunderstanding some part of the behavior here

@NickCraver
Copy link
Collaborator

@usjpin Anything that relies on endpoints overall: subscriptions and restorations, server selections that are in-flight, etc. - in general this adds a lot of complexity to those scenarios to even think about, to the point I'd say "maybe this is where you'd want a fork of the library", if not using a stable proxy or routable endpoint like most common scenarios (e.g. a load balancer).

@NickCraver
Copy link
Collaborator

Unfortunately we found another area this will cause problems: async completions. Though we're fixing this in #2413 for current paths, this would create another orphan scenario where a heartbeat would neither root nor complete any pending messages or async continuations on orphaned bridges from the prior endpoints. Given everything this causes (largely why it doesn't exist today) my vote would be: we don't accept this API addition.

@mgravell
Copy link
Collaborator

Just to be explicit, @NickCraver - your thoughts here are around reassigning Endpoints in a way that removes some, yes?

@NickCraver
Copy link
Collaborator

NickCraver commented Mar 28, 2023

@mgravell yeah that happening in several places is not awesome - we should either find a way to do this in ConfigurationOptions that allows you to change them, or not do this IMO. The fact it's not mutable there but then changes here via another API is odd as a starting point, but complicating factors like the heartbeat pathing, continuations, whether DNS is resolved and how we improve that for re-connects...this locks us in and reduces options.

Not my primary concern, but it's also confusing at best at the 2-level Sentinel scenario where we're returning an inner-connection and hiding most of the sausage there today.

@NickCraver
Copy link
Collaborator

We chatted about this in the sync today. There are a lot of problems enabling this in this way and in general, but overall if we make endpoints mutable after startup, it'll be via ConfigurationOptions so they're changed like everything else, rather than a special API which both restricts our options and introduces ambiguity about what to do and which one wins (e.g. calling this proposed API on a ConnectionMultiplexer that was configured with options...that were also used to make others).

We're not dismissing the use case (I'd like to actually preserve that in an issue and add others - a few overlap here), but the API itself isn't the way we'd go about enabling it. Thanks a ton for both the work and the info here...we'd like to solve this problem, just need to come at it a bit differently.

@NickCraver NickCraver closed this Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants