Skip to content

Rule Manager: Improve CircuitBreaker rule loading API (#236) #257

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimDu
Copy link

@TimDu TimDu commented Sep 27, 2020

Describe what this PR does / why we need it

PR per issue #236

Does this pull request fix one issue?

Add fixes to solve issue #236

Describe how you did it

Add logic to check for existing rule changes in this API and return as the first value
Put recover method in one place so that all panics can be caught and assigned to the second value
Failed rules will be returned to caller as the third value
Add duplicate rule checker so that all loaded rules will be unique

Describe how to verify it

Added more UT cases to verify it

Special notes for reviews

Also Add CB Strategy Enumeration upper boundary.
Modify API signature

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2020

CLA assistant check
All committers have signed the CLA.

@louyuting
Copy link
Collaborator

Could you please fix CI?

@codecov-commenter
Copy link

Codecov Report

Merging #257 into master will increase coverage by 0.66%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   43.06%   43.73%   +0.66%     
==========================================
  Files          79       79              
  Lines        3994     4020      +26     
==========================================
+ Hits         1720     1758      +38     
+ Misses       2009     2003       -6     
+ Partials      265      259       -6     
Impacted Files Coverage Δ
core/circuitbreaker/rule_manager.go 54.00% <86.04%> (+12.62%) ⬆️
core/circuitbreaker/rule.go 36.66% <100.00%> (+6.66%) ⬆️
ext/datasource/helper.go 55.78% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e8f5a0...f2ae09e. Read the comment docs.

@TimDu
Copy link
Author

TimDu commented Sep 27, 2020

Thanks @louyuting for your update. I did only send out changes on CircuitBreaker module since there are multiple changes to Flow module in the latest branch. Would love to discuss and help on other modules. Please let me know if any concerns on current implementation.

@louyuting
Copy link
Collaborator

Thanks @louyuting for your update. I did only send out changes on CircuitBreaker module since there are multiple changes to Flow module in the latest branch. Would love to discuss and help on other modules. Please let me know if any concerns on current implementation.

That's great. I will review this PR. Because of holiday, the response might be later.

@louyuting louyuting added this to the 1.1.0 milestone Dec 7, 2020
@louyuting louyuting added area/circuit-breaking Issues or PRs related to circuit breaking kind/enhancement Category issues or PRs related to enhancement area/rule-manager Issues or PRs related to rules manager labels Dec 7, 2020
@louyuting louyuting changed the title Improve CircuitBreaker rule loading API (#236) Rule Manager: Improve CircuitBreaker rule loading API (#236) Dec 7, 2020
@louyuting louyuting modified the milestones: 1.1.0, 1.2.0 Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/circuit-breaking Issues or PRs related to circuit breaking area/rule-manager Issues or PRs related to rules manager kind/enhancement Category issues or PRs related to enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants