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

Allow a Processing Strategy to be injected and configured via new startup extension methods #180

Merged
merged 2 commits into from
May 25, 2021

Conversation

nick-cromwell
Copy link
Contributor

Fixes #83

This is a WIP and still needs supporting tests, but wanted to open this for discussion.

This takes after the work of @simonhaines in his atomic-lua branch and adds a ProcessingStrategy for AsyncKeyLocker and also StackExchangeRedis allowing both to function while being mostly backwards compatible. The one exception is the need to register the IProcessingStrategyFactory with the ServiceProvider - which has been simplified with the new extension methods. The IProcessingStrategyFactory isn't perfect, but it avoids the need for more complicated injection strategies or container like Autofac.

This would require a major version bump.

@cristipufu
Copy link
Collaborator

Hi @nick-cromwell, great stuff. As this is such a major change, I would like to roll it out incrementally and split this PR into multiple PRs and maybe even multiple projects & NuGet packages (eg: AspNetCoreRateLimit.Redis - to avoid unwanted dependencies).

So I think that these should be the steps moving forward:

Thanks for your contribution!

@nick-cromwell
Copy link
Contributor Author

That sounds like a good plan, give me a few days I'll get those out.

…rategy; remove StackExchange code and references
@nick-cromwell nick-cromwell changed the title Add StackExchangeRedis atomic Lua script support and refactor to supp… Allow a Processing Strategy to be injected and configured via new startup extension methods Dec 19, 2020
@nick-cromwell nick-cromwell marked this pull request as ready for review December 19, 2020 19:14
@nick-cromwell
Copy link
Contributor Author

@cristipufu Happy new year. This ready to be merged whenever you're available and I have tested the Redis Lua script processor in a separate branch. I'll open a PR for that once we've bumped the major version.

@simonhaines
Copy link

simonhaines commented Jan 8, 2021

Injecting the processing strategy is a much cleaner approach than the one used in my fork, and fits the overall design of this library far better. I prefer this over my own work, and will update/redirect my fork when this is merged. Thank you @nick-cromwell for taking the time to integrate this, and thank you @cristipufu for the library in the first place!

@nick-cromwell
Copy link
Contributor Author

@cristipufu Can we version bump and merge this in the near future. The concurrency issue is still receiving a fair amount of attention. #83

@cristipufu cristipufu merged commit da8f0cd into stefanprodan:master May 25, 2021
@cristipufu
Copy link
Collaborator

@nick-cromwell many thanks for your contribution! Feel free to add the AspNetCoreRateLimit.Redis extension project with a new PR and I'll publish it as a new NuGet package.

I renamed the startup extensions & added a new one with a generic parameter to easily change the processing strategy.

Thanks!

cc: @stefanprodan can you please add @nick-cromwell as a contributor to this project?

@nick-cromwell
Copy link
Contributor Author

Project was added in PR #210. Happy to help!

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.

Distribute cache concurrency issue
3 participants