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

RedisCache provider #84

Closed
wants to merge 3 commits into from
Closed

RedisCache provider #84

wants to merge 3 commits into from

Conversation

nightBaker
Copy link

RedisCacheTicketManager implementation for ticket storage in .NET 4.5 based on CachingServer.Redis

@phantomtypist
Copy link
Contributor

Cool @nightBaker!

However, we were aiming to have these distributed cache providers implemented as separate installable NuGet packages: #61

This way people can opt-in to the different add-ons they want instead of getting everything built-in.... makes things lightweight that way. Think of the scenario where someone only wanted the plain vanilla CAS client, but had to end up pulling in a bunch of other dependencies like Memcached and Redis because they were compiled in by us.

@TheHokieCoder
Copy link
Contributor

@phantomtypist beat me to it 😄, but I'll leave my answer anyways!

@nightBaker Love the effort to get a new service ticket manger implement for this project! However, per issue #61, I believe that the idea/plan is to have all new implementations done as separate .NET/NuGet packages that can be added to a project to extend the functionality of DotNetCasClient. By adding this provider directly to DotNetCasClient, we would be adding functionality that is not useful to the general .NET population, and therefore adding additional testing & debugging work to the main project. Per @phantomtypist's comments in the related issue, it'd be best to implement the new service ticket manager as it's own project and class library that can be published as separate, related NuGet package. Once it has been fully tested and published, we can update the documentation of this project to reflect its availability for those who may want that functionality.

This is my understanding, and my personal opinion, so I'd like for other project maintainers to chime in if possible.

As a further note, when working on my own project for a service ticket manager backed by Microsoft SQL Server, I found that there is the need to support significant configuration options (e.g. connection strings, application name) and have considered creating its own configuration section similar to how DotNetCasClient does. I haven't yet looked over the configuration requirements for your RedisCache implementation, so perhaps you will run into a similar problem when you can't modify the CasClientConfiguration class. If so, this would be good to discuss in issue #61.

@phantomtypist
Copy link
Contributor

@TheHokieCoder agreed. I'll loop @mmoayyed so we can get another GitHub repo/project created for the Redis cache provider implementation.

I envision it the same where in that repo it will have it's own solution/project and be itself a class library. That class library will generate a NuGet package along with it's own configuration file transformations.

My OCD in me doesn't like the "dotnet-cas-client" repo name. In an ideal world I would have preferred "DotNetCasClient", but that ship kind of sailed :-/

Naming is always hard... so I see a few options for the repo name of the new Redis cache provider:

  1. "dotnet-cas-client-redis"
  2. "dotnet-cas-client.redis"
  3. "DotNetCasClient.Redis" <= This should be what the actual NuGet package ID will is.

@phantomtypist
Copy link
Contributor

@nightBaker so here is what we are going to do. I think we are going to end up creating a separate repo for each tertiary NuGet package. I'll scaffold the repo (project, build scripts, CI pipeline, NuGet package feeds, etc.). Then I'll have you port this specific code over to that project.

@phantomtypist
Copy link
Contributor

@nightBaker I didn't look too deep at your code, but I was wondering what features you were using in CacheFramework.Redis that are not available in StackExchange.Redis

CacheFramework.Redis appears to be a fork with some added features, but if we aren't using any of those features I'd rather rely on the more well known StackExchange.Redis.

One other thought I have is you should probably open up an issue (if it doesn't already exist) for each chunk of work you want to contribute (i.e. this Redis cache provider.) In that issue discuss what you want to do and how you are thinking of tackling it. This opens things up for discussion. I'd rather you, nor anybody else, run off to code in the dark and submit a PR only to find out after a discussion in the PR that leads to more changes. I want to make sure you use your time in the most efficient manner possible.

That reminds me.... I guess I should make a "Contributing Guidelines" page for this project.

@nightBaker
Copy link
Author

@phantomtypist , I've used Redis data types as .NET collections feauture of CacheFramework.Redis, especially hash for storing tickets data.

So do I have to open issue in this repository or I have to wait until you create new repo and open issue in new repo ?

@nightBaker nightBaker closed this Apr 19, 2018
@phantomtypist
Copy link
Contributor

We are going to create a new repo called dotnet-cas-client-redis and we can put this code in there.

@phantomtypist
Copy link
Contributor

@nightBaker The PR looks good a cursory glance. Will have to build and test it once we make the new repo (give it a few weeks to get that all together.)

@phantomtypist
Copy link
Contributor

@nightBaker I just wanted to give you an update. We've created the repo for this although it's blank right now. We'll scaffold the project, solution and build scripts in there very soon and then you can retarget your PR to the new repo (https://github.com/apereo/dotnet-cas-client-redis).

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