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

Injecting custom instance of IConnectionMultiplexer #106

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

footcha
Copy link

@footcha footcha commented Aug 10, 2018

This PR contains initial implementation of #105.
Your feedback is welcome.
Thank you for your work!
BR
Petr

…or handling custom lifecycle of StackEchange.IConnectionMultiplexer.
@msftclas
Copy link

msftclas commented Aug 10, 2018

CLA assistant check
All CLA requirements met.

@footcha footcha changed the title Connection multiplexer factory Injecting custom instance of IConnectionMultiplexer Aug 10, 2018
@SiddharthChatrolaMs
Copy link
Contributor

SiddharthChatrolaMs commented Aug 20, 2018

@footcha I haven't got time to look into this yet. Once I am done with my current task, I will verify this and update this thread. Sorry for the delay

@footcha
Copy link
Author

footcha commented Aug 23, 2018

@SiddharthChatrolaMs Is there any progress in a code review? Any estimate when you will be able to provide a feedback?
Thank you,
Petr

@JonCole
Copy link
Contributor

JonCole commented Aug 23, 2018

Our team is pretty slammed right now and we probably wont get to this right away. One thing that jumps out at me is some concern over the fact that you are using an interface for the factory, which makes future changes/behaviors around controlling the multiplexer challenging. At the very least, I would think we should consider an abstract class instead of the interface, which would allow us to add new, virtual methods if the need arises. However, if you have other suggestions, those would be great to hear.

@footcha
Copy link
Author

footcha commented Aug 24, 2018

@JonCole Thank you for a response. I understand your suggestion to replace an interface-based factory with an abstract class. I will do that.

@JonCole
Copy link
Contributor

JonCole commented Aug 24, 2018

@footcha - one of the other things that I am concerned about with this PR is the fact that this effectively exposes some internal abstractions that aren't necessarily designed sufficiently to be ready for public consumption. For instance, before accepting this PR, I would want to think about how far we think we want to take the abstraction around the connection. Do we want to allow app developers to control the calls to the multiplexer? Do we want to let developers replace StackExchange.Redis entirely with some other client library? I think we need to answer some of these longer term plans around extensibility before we can decide if your PR has the right long-term design.

Unfortunately, our team doesn't currently have the cycles to spend time digging into the longer term extensibility plans, thus the desire on my end to wait a bit longer before acting on this PR.

Happy to hear thoughts/feedback from you and others as well on this topic...

@footcha footcha closed this Oct 26, 2018
@footcha footcha deleted the ConnectionMultiplexerFactory branch October 26, 2018 11:06
@footcha footcha restored the ConnectionMultiplexerFactory branch October 26, 2018 11:08
@footcha footcha reopened this Oct 26, 2018
@jvilimek
Copy link

Hello, has someone already time to finish review/merge? @JonCole ? @SiddharthChatrolaMs

@aravindyeduvaka
Copy link

Hey sorry about the delay but our team has been pretty busy.
@JonCole and I discussed this but we don't see a strong need for this since configuration settings allow users to customize the settings that most care about. Unless you feel that there are strong reasons to do otherwise, we are leaning towards saying no to this PR.

@stanleysmall-microsoft stanleysmall-microsoft changed the base branch from master to main May 11, 2022 20:34
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.

6 participants