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

Nuget me up! #1

Open
Jetski5822 opened this issue Apr 11, 2019 · 7 comments
Open

Nuget me up! #1

Jetski5822 opened this issue Apr 11, 2019 · 7 comments

Comments

@Jetski5822
Copy link

Jetski5822 commented Apr 11, 2019

This package is awesome - some really goo ideas, fancy throwing it up on to nuget?

@vany0114
Copy link
Owner

vany0114 commented Apr 12, 2019

@Jetski5822 thanks appreciate it!

Regarding the nuget, maybe this might be a new project for Polly.Contrib. We might have several nuget packages to handle different resilience strategies in a generic way, such as Azure SQL Databases, Azure Service Bus, etc. Also, we would need to make them extensible in order to people can inject additionals strategies to the builder (and a lot of enhancements as well, I didn't write this guy thinking about a nuget tho 😅 ), etc what do you think @reisenberger?

@reisenberger
Copy link

reisenberger commented Apr 13, 2019

@vany0114 I'll dig in and have a look!

@reisenberger
Copy link

reisenberger commented Apr 16, 2019

Hi @vany0114 . It is great to see everything you are doing around Polly! I dug into the code (and blog post), but quickly, so, please, feel free to say if I have misunderstood anything.

It looks broadly as if this project does three different kinds of thing around Polly:

(1) Specific helper methods around SQL errors (and suggestion we could do similar for eg Azure Service Bus; other services).
(2) New builder syntax for the policies / policywrap
(3) some combining of the sync and async configuration syntax.


If we wanted to take some of this into a/ Polly-Contrib project/s, it might be better (and cleaner for users to understand) to tackle some of these ^ concerns separately:

(1) Specific helper methods around SQL errors (or similar for other downstream components)

These could be great additions to Polly-Contrib. A similar concept to Polly.Extensions.Http, which contains "off-the-shelf" configuration for handling standard http-call errors. Polly.Extensions.Sql or Polly.Extensions.AzureServiceBus (etc) could make great contrib projects. They might (like Polly.Extensions.Http) be limited just to the Handle-clause parts of the syntax, unless I have missed some other angle.

(2) New builder syntax for the policies

I like the new builder syntax, but I have different goals / think we have bigger challenges for a syntax overhaul for Polly. The biggest challenge I see is the proliferation of overloads - for example the large number of Retry policy overload options - the fact that there are overloads for many different possible combinations of parameters. This design (which I inherited on the project) is preventing Polly growth. I would like to change the Polly policy configuration syntax (major breaking change!) to something like Policy.Handle<>(/* etc */).Retry(Action<RetryConfiguration>) ... in other words, similar to much of the .NET Core configuration syntax. This Action<RetryConfiguration> pattern is much more open for extension.

I see this ^ as the most pressing syntax problem to solve for Polly.

(3) some combining of the sync and async configuration syntax

Despite looking hard at whether we could merge sync/async policies, the fact that users want and need async delegate hooks for async policies (and if we don't supply this, it leaves users to fall into the pit of failure with those nasty async void problems) ... means probably we will keep the sync/async split in Polly.

(Aside: If an approach such as .Retry(Action<RetryConfiguration>) discussed above was adopted, it might be possible to achieve some commonality of code by having SyncRetryConfiguration : RetryConfiguration and also AsyncRetryConfiguration : RetryConfiguration ....)

(I might have misunderstood what you have done around sync/async tho.)


Please don't be discouraged if you want to nuget this; I am just trying to answer the question which you and @Jetski5822 raised 🙂 , and hopefully provide some useful input!
No obligation also to follow any of these suggestions ...

Thanks.

@vany0114
Copy link
Owner

vany0114 commented Apr 17, 2019

First things first, @reisenberger thanks for taking the time to dig into the code/blog post I really appreciate it!

Well, the idea behind this was to have a common way to build and manage different resilience strategies thru Polly. I needed to handle Azure SQL databases errors, and most of the times I was using the same combinations of policies (that's why the builder has a method called WithDefaultPolicies). So, the main idea was to build specialize resilience strategies using Polly since every strategy will depend on the component you want to handle, it's not the same, a resilient strategy for Azure SQL databases than for Azure Service Bus.

Regarding the syntax, I just thought in a friendly, flexible, intuitive and easy way to use and build the strategy (like Polly does) but as I said earlier, I wasn't thinking to make a library or something like that, I was just encouraging people to think in that way then they could make their specialized strategies given their use cases, so I just purposed a cool way to do that thru a builder.

Regarding combining of the sync and async configuration syntax, actually, I have two separate implementations for both, sync and async policies that I use thru UseAsyncExecutor or UseSyncExecutor, like this:

var resilientAsyncStrategy = builder
    .UseAsyncExecutor()
    .WithDefaultPolicies()
    .Build();

But it really doesn't matter, as I said, that syntax was only an example of one approach I wanted to show.

Wrapping up, the main idea here is that would be nice having specialized (and common) resilience strategies for a different kind of components, such as Azure SQL databases, Azure Service Bus, etc like you did with Polly.Extensions.Http. I really like that idea because it's just matter of download the nuget and set it up and that's it, you don't have to build that strategy over and over again. Having said that I'd really like to do something like you said earlier:

Polly.Extensions.Sql or Polly.Extensions.AzureServiceBus (etc) could make great contrib projects. They might (like Polly.Extensions.Http) be limited just to the Handle-clause parts of the syntax.

^ that's the main idea behind this.

So, please, let me know your thoughts and if you feel we can make this I'll be more than happy to do it after we release Simmy!

Thanks again.

@reisenberger
Copy link

reisenberger commented Apr 17, 2019

Thanks @vany0114 . It sounds like we have a similar perspective. Also, I should be clear: this project is a great example of how people can build their own thing around Polly to suit their needs! And perfect to put all in one project for that. When I was saying "to tackle these concerns separately", that was only in the context of @Jetski5822 's question "Should we nuget this up?" (and perhaps in Polly-Contrib). For nuget release in Polly-Contrib, I would tackle the aspects separately.

Thanks again for all the awesome things you are doing around Polly! 👍 🙏

@vany0114
Copy link
Owner

vany0114 commented Apr 17, 2019

So @reisenberger. let me know if you'd like to go further with these ideas on Polly-Contrib 😃

@Jetski5822
Copy link
Author

Jetski5822 commented Apr 17, 2019

I currently pulled the whole project in locally, and its working great. Really loving it, if we could get this in to polly contrib is some manner, that would be great!

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

No branches or pull requests

3 participants