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.

Sign up for GitHub

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

Jump to bottom

#1844 More open customization of Polly use #1845

Merged
merged 8 commits into from
Feb 23, 2024
Merged

#1844 More open customization of Polly use #1845

merged 8 commits into from
Feb 23, 2024

Conversation

RaynaldM
Copy link
Collaborator

@RaynaldM RaynaldM commented Dec 7, 2023 •
edited by raman-m
Loading

In certain contexts, we need to be able to fully tune the way Polly is used for timeouts and circuit-breakers, but not only that.
With this in mind, I'm proposing in this PR to open up the use of AddPolly by adding :

public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder, Dictionary<Type, Func<Exception, Error>> errorMapping)
     where T : class, IPollyQoSProvider<HttpResponseMessage> =>
     AddPolly<T>(builder, GetDelegatingHandler, errorMapping);

 public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder, QosDelegatingHandlerDelegate delegatingHandler)
     where T : class, IPollyQoSProvider<HttpResponseMessage> =>
     AddPolly<T>(builder, delegatingHandler, ErrorMapping);

 public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder)
     where T : class, IPollyQoSProvider<HttpResponseMessage> =>
     AddPolly<T>(builder, GetDelegatingHandler, ErrorMapping);

Thanks to this, we will be able to use our own implementations of IPollyQoSProvider<HttpResponseMessage>, QosDelegatingHandlerDelegate and Dictionary<Type, Func<Exception, Error>>

@RaynaldM RaynaldM requested a review from raman-m December 7, 2023 14:03
@RaynaldM RaynaldM linked an issue Dec 7, 2023 that may be closed by this pull request
More open customization of Polly use #1844
Closed
@RaynaldM RaynaldM added feature A new feature small effort Likely less than a day of development effort. 2023 Annual 2023 release labels Dec 7, 2023
@RaynaldM RaynaldM added this to the December'23 milestone Dec 7, 2023
@RaynaldM RaynaldM added the proposal Proposal for a new functionality in Ocelot label Dec 8, 2023
@raman-m
Copy link
Member

raman-m commented Dec 11, 2023

@ggnaegi FYI
Easy to review, but it is marked for Dec'23 release...

@raman-m
Copy link
Member

raman-m commented Dec 11, 2023

@RaynaldM Thanks for the PR! New extensions will be useful.
Why not to test them somehow?

@raman-m raman-m added Jan'24 January 2024 release and removed 2023 Annual 2023 release labels Feb 17, 2024
@raman-m raman-m modified the milestones: Annual 2023, January'24 Feb 17, 2024
@raman-m raman-m force-pushed the 1844-more-open-customization-of-polly-use branch from 4e92be2 to 9f7a724 Compare February 17, 2024 12:53
raman-m
raman-m reviewed Feb 17, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because new extensions are public... we need XML developer's documentation.

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs Outdated
using Polly.CircuitBreaker;
using Polly.Timeout;

namespace Ocelot.Provider.Polly;

public static class OcelotBuilderExtensions
{
private static readonly Dictionary<Type, Func<Exception, Error>> ErrorMapping = new Dictionary<Type, Func<Exception, Error>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why private? Let's make it public.
Let's rename to DefaultErrorMapping

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RaynaldM Can we inject custom mapper somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public is useless, this default error mapping is only use in this class, and I can not surcharged because it is a static.
I can let it public if you want, but it is not a good practice to expose useless methods or properties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can inject a custom mapper in line 42 (that what we are using currently in oiur implementation)

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs Outdated Show resolved Hide resolved
@raman-m
Copy link
Member

raman-m commented Feb 17, 2024

@ggnaegi Please review!

@raman-m raman-m added the high High priority label Feb 17, 2024
@raman-m
Copy link
Member

raman-m commented Feb 19, 2024 •
edited
Loading

But the build is still 🔴

@raman-m raman-m self-requested a review February 19, 2024 11:46
@raman-m
Copy link
Member

raman-m commented Feb 23, 2024

@RaynaldM Is your development finished here?

@raman-m raman-m force-pushed the 1844-more-open-customization-of-polly-use branch from 9006de1 to 7189c06 Compare February 23, 2024 10:40
@raman-m
Copy link
Member

raman-m commented Feb 23, 2024 •
edited
Loading

🤕

@raman-m
Copy link
Member

raman-m commented Feb 23, 2024

If you love to press GitHub buttons then I'm letting you to finish your PR.
Please fix compilation errors!

@raman-m
Copy link
Member

raman-m commented Feb 23, 2024

My understanding this PR should be merged first before #1914 ... right?

@RaynaldM
Copy link
Collaborator Author

My understanding this PR should be merged first before #1914 ... right?

Exactly, it is an intermediate step

@RaynaldM RaynaldM merged commit f88bd51 into develop Feb 23, 2024
2 checks passed
@RaynaldM RaynaldM deleted the 1844-more-open-customization-of-polly-use branch February 23, 2024 13:16
@raman-m raman-m restored the 1844-more-open-customization-of-polly-use branch February 23, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m approved these changes

@binarymash binarymash Awaiting requested review from binarymash

@jps jps Awaiting requested review from jps

@geffzhang geffzhang Awaiting requested review from geffzhang

@philproctor philproctor Awaiting requested review from philproctor

@anktsrkr anktsrkr Awaiting requested review from anktsrkr

@sharpn sharpn Awaiting requested review from sharpn

@abooraja abooraja Awaiting requested review from abooraja

@wast wast Awaiting requested review from wast

@yangzhongke yangzhongke Awaiting requested review from yangzhongke

@thiagoloureiro thiagoloureiro Awaiting requested review from thiagoloureiro

Assignees

@RaynaldM RaynaldM

Labels
feature A new feature high High priority Jan'24 January 2024 release proposal Proposal for a new functionality in Ocelot small effort Likely less than a day of development effort.
Projects
None yet
Milestone
January'24
Development

Successfully merging this pull request may close these issues.

More open customization of Polly use
2 participants
@RaynaldM @raman-m