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

#908 Quality of Service - Configuration #1279

Merged
merged 15 commits into from
Sep 22, 2023
Merged

#908 Quality of Service - Configuration #1279

merged 15 commits into from
Sep 22, 2023

Conversation

DanHarltey
Copy link
Contributor

@DanHarltey DanHarltey commented Jul 1, 2020 •
edited by raman-m
Loading

Fixes #908

Fixing an issue where only setting timeout value in QoS would result in a exception getting thrown.

Proposed Changes

  • Added small check to see if there is only Policy
  • Update a unit test to trigger the behavior

@LukePammant LukePammant mentioned this pull request Oct 1, 2021
Closed
eddex
eddex previously approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@eddex eddex left a comment

Choose a reason for hiding this comment

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

lgtm

@somasundar1997 somasundar1997 mentioned this pull request Oct 20, 2022
Closed
@somasundar1997
Copy link

Hi when can I get this release.

@somasundar1997
Copy link

@eddex when I can expect this to get merged with the main branch

@eddex
Copy link
Contributor

eddex commented May 16, 2023

@eddex when I can expect this to get merged with the main branch

I'm not a maintainer of Ocelot, just a user that wants to get this fixed too. I think @raman-m might be able to merge it.

@raman-m
Copy link
Member

raman-m commented May 16, 2023

@eddex when I can expect this to get merged with the main branch

I'm not a maintainer of Ocelot, just a user that wants to get this fixed too. I think @raman-m might be able to merge it.

@eddex Marco,
That is incorrect! I have no right to merge PRs! Only repository owners can merge to develop branch. So, this is Tom Pallister only!
All Ocelot Core team members cannot merge to develop branch, because it is protected! But team members can drive the process of Code Review, and give an approvals. Nothing more! Sorry!

Also. I'm not going even to review this PR because of invalid base (target) branch. So, we don't deliver fixes to master! We deliver bug fixes and new features to develop branch only! I guess Tom had changed development process (git flow) maybe a few years ago. So, master (main) was default branch in the past (2-3 years ago). After that Tom has made develop as default branch.

@raman-m raman-m added invalid Not actually an issue needs validation Issue has not been replicated or verified yet waiting Waiting for answer to question or feedback from issue raiser labels May 16, 2023
@raman-m
Copy link
Member

raman-m commented May 16, 2023

@DanHarltey Hi Dan!

Requesting to master is wrong!
You can request PRs to develop branch only!

Please, change the base (target) branch!
I would recommend to pull latest commits from base:develop and please apply your changes on the top commit!

Pay attention that CI pipelines are working for main and develop branches only!
I have to check CI pipeline status also before starting code review...
Also, I have no right to change PR target/base branch. So, only PR author can edit the PR!

@raman-m raman-m marked this pull request as draft May 16, 2023 13:19
@raman-m raman-m linked an issue May 20, 2023 that may be closed by this pull request
Quality of Service - Configuration #908
Closed
@raman-m raman-m changed the title Fix issue 908 #908 Quality of Service - Configuration May 20, 2023
@raman-m raman-m changed the base branch from master to develop May 20, 2023 15:01
@raman-m raman-m dismissed eddex’s stale review May 20, 2023 15:01

The base branch was changed.

@raman-m raman-m marked this pull request as ready for review May 20, 2023 15:05
@raman-m raman-m removed invalid Not actually an issue waiting Waiting for answer to question or feedback from issue raiser labels May 20, 2023
@raman-m raman-m requested review from raman-m and eddex May 20, 2023 15:08
eddex
eddex previously approved these changes May 24, 2023
View reviewed changes
raman-m
raman-m previously approved these changes May 25, 2023
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. 

+ Approved

@raman-m raman-m requested a review from eddex May 25, 2023 19:18
@raman-m
Copy link
Member

raman-m commented May 25, 2023 •
edited
Loading

@eddex Hi Marco!
Sorry for stale review!

Could you review old and my changes once again please?

Please note that real bug fix is in try-block of the SendAsync method of the PollyCircuitBreakingDelegatingHandler class: Lines 28-41

And the core of the bug fix is in lines L33-L35 which was proposed by @DanHarltey in his solution from initial commit 91eed16d96cf13e97d6215bcfb48f1ddf5d0fadd.
So, the Policy.WrapAsync(policies) generates exception if policies collection has less than 2 elements (0 and 1). The fix takes this condition into account.
Finally, we can apply wrapping operation for policies collections with 2 elements or more.

Also, I have added unit tests as the PollyCircuitBreakingDelegatingHandlerTests class to cover the cases above.

I would appreciate your code review!

@raman-m raman-m added the waiting Waiting for answer to question or feedback from issue raiser label May 25, 2023
raman-m
raman-m previously approved these changes Aug 2, 2023
View reviewed changes
@raman-m
Copy link
Member

raman-m commented Aug 2, 2023

@DanHarltey commented on July 21

Dan, JFYI
Because we have synced develop branch, I've successfully rebased feature branch onto ThreeMammals:develop.

Thanks for updating of develop branch!

@raman-m raman-m requested review from RaynaldM and wast August 4, 2023 11:20
RaynaldM
RaynaldM previously approved these changes Aug 7, 2023
View reviewed changes
src/Ocelot.Provider.Polly/PollyCircuitBreakingDelegatingHandler.cs Outdated
.WrapAsync(_qoSProvider.CircuitBreaker.Policies)
.ExecuteAsync(() => base.SendAsync(request, cancellationToken));
var policies = _qoSProvider.CircuitBreaker.Policies;
if (policies.Any())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (policies.Any())
if (!policies.Any()) return await base.SendAsync(request, cancellationToken);

Copy link
Member

Choose a reason for hiding this comment

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

Fixed by RaynaldM code review

src/Ocelot.Provider.Polly/PollyCircuitBreakingDelegatingHandler.cs Outdated Show resolved Hide resolved
@raman-m raman-m self-requested a review August 14, 2023 12:02
@raman-m raman-m dismissed stale reviews from RaynaldM and themself via 59df4f4 August 14, 2023 12:10
@raman-m raman-m requested a review from RaynaldM August 14, 2023 12:27
@raman-m
Copy link
Member

raman-m commented Aug 18, 2023

@TomPallister
This PR is ready to merge!

@raman-m raman-m merged commit 21bc4a4 into ThreeMammals:develop Sep 22, 2023
@raman-m raman-m mentioned this pull request Oct 3, 2023
Merged
@raman-m raman-m added the QoS Ocelot feature: Quality of Service label Oct 25, 2023
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

@eddex eddex eddex approved these changes

@RaynaldM RaynaldM RaynaldM approved these changes

@TomPallister TomPallister Awaiting requested review from TomPallister

@wast wast Awaiting requested review from wast

Assignees

@DanHarltey DanHarltey

Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug QoS Ocelot feature: Quality of Service
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quality of Service - Configuration
5 participants
@DanHarltey @somasundar1997 @eddex @raman-m @RaynaldM