Fix issue 673 Make the value of RT_MAX_EXCEED_N in DegradeRule configurable #789

linlinisme · 2019-05-25T04:22:41Z

Describe what this PR does / why we need it

Make the value of RT_MAX_EXCEED_N in DegradeRule configurable

Does this pull request fix one issue?

fix #673

Describe how you did it

DegradeRule add a new filed rtMaxExceedN to replace RT_MAX_EXCEED_N

Describe how to verify it

Configure rtMaxExceedN to see if it takes effect.

Special notes for reviews

using the LongAdder rather than AtomicInteger to Provides better performance

…able

…able make some test

merge master

codecov-io · 2019-05-25T04:28:23Z

Codecov Report

Merging #789 into master will increase coverage by 0.16%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##             master     #789      +/-   ##
============================================
+ Coverage     41.71%   41.87%   +0.16%     
- Complexity     1376     1386      +10     
============================================
  Files           304      304              
  Lines          8764     8778      +14     
  Branches       1182     1184       +2     
============================================
+ Hits           3656     3676      +20     
+ Misses         4662     4653       -9     
- Partials        446      449       +3

Impacted Files	Coverage Δ	Complexity Δ
...a/com/alibaba/csp/sentinel/node/StatisticNode.java	`55.68% <100%> (ø)`	`27 <0> (ø)`	⬇️
.../csp/sentinel/slots/block/degrade/DegradeRule.java	`62.1% <57.89%> (+6.54%)`	`23 <3> (+6)`	⬆️
...a/csp/sentinel/slots/statistic/base/LeapArray.java	`70.29% <0%> (+2.97%)`	`34% <0%> (+1%)`	⬆️
...alibaba/csp/sentinel/slots/block/AbstractRule.java	`58.62% <0%> (+10.34%)`	`14% <0%> (+3%)`	⬆️

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 062385f...11cbc7f. Read the comment docs.

…sue-673

sczyh30 · 2019-05-27T06:15:30Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/degrade/DegradeRule.java

@@ -55,7 +55,7 @@
 */
 public class DegradeRule extends AbstractRule {

-    private static final int RT_MAX_EXCEED_N = 5;
+    private int rtMaxExceedN = RuleConstant.DEGRADE_GRADE_RT_MAX_EXCEED_N;


The RT_MAX_EXCEED_N has two meanings actually:

For RT mode, it indicates the minimum number of consecutive slow requests that can trigger RT circuit breaking.

For exception ratio mode, it indicates the minimum number of requests (in an active statistic time span) that can trigger circuit breaking.

So maybe we need two attributes here? And the attribute name might need to be discussed.

hi, @sczyh30

DegradeRule rule = new DegradeRule(); rule.setResource(KEY); // set limit exception ratio to 0.1 rule.setCount(0.1); rule.setGrade(RuleConstant.DEGRADE_GRADE_EXCEPTION_RATIO); rule.setTimeWindow(10); rule.setRtMaxExceedN(20); rules.add(rule);

The downgrade rule can only be set to one mode at present. rt mode and exception ratio mode Only one can be selected. The RT_MAX_EXCEED_N attribute can represent different meanings in different modes. So I think that using only one attribute is ok. and you ?

They're indeed two different things, so only one attribute might be confusing. I think we can make it two attributes:

rtSlowRequestAmount for minimum number of consecutive slow requests that can trigger RT circuit breaking.

minRequestAmount for the minimum number of requests (in an active statistic time span) that can trigger circuit breaking.

What do you think of it? @CarpenterLee

Any progress?

two attribute may be better

…ace rtMaxExceedN

sczyh30 · 2019-06-05T22:12:41Z

Could you please also update the test cases for these two new attributes?

linlinisme · 2019-06-06T03:59:34Z

Could you please also update the test cases for these two new attributes?

@sczyh30 Dear sczyh30. I have updated test cases. but i find another potential problem: may be the hashCode() method should consider the problem of numerical out of bounds.

sczyh30 · 2019-06-10T12:28:52Z

Could you please also update the test cases for these two new attributes?

@sczyh30 Dear sczyh30. I have updated test cases. but I find another potential problem: maybe the hashCode() method should consider the problem of numerical out of bounds.

Maybe overflow isn't a problem for hashCode() as long as it could achieve good dispersion and uniqueness.

linlinisme · 2019-06-11T03:51:48Z

Could you please also update the test cases for these two new attributes?

@sczyh30 Dear sczyh30. I have updated test cases. but I find another potential problem: maybe the hashCode() method should consider the problem of numerical out of bounds.

Maybe overflow isn't a problem for hashCode() as long as it could achieve good dispersion and uniqueness.

ok, I see . Would you view what i had commited is ok or not?

...emo-basic/src/main/java/com/alibaba/csp/sentinel/demo/degrade/ExceptionRatioDegradeDemo.java

sczyh30 · 2019-06-12T14:58:42Z

sentinel-core/src/test/java/com/alibaba/csp/sentinel/slots/block/degrade/DegradeTest.java

+        // Will true. While totalQps > minRequestAmount and exceptionCount < minRequestAmount
+        when(cn.totalQps()).thenReturn(21d);
+        when(cn.exceptionQps()).thenReturn(19d);
+        assertTrue(rule.passCheck(context, node, 1));


There are some problems in the test case. Here successQps is not provided, so it's by default 0, which does not match the actual relation:

in the same aligned statistic time window, success (aka. completed count) = exception count + non-exception count (realSuccess)

There are some problems in the test case. Here successQps is not provided, so it's by default 0, which does not match the actual relation:

in the same aligned statistic time window, success (aka. completed count) = exception count + non-exception count (realSuccess)

Do you mean ?

double realSuccess = success - exception; if (realSuccess <= 0 && exception < minRequestAmount) { return true; }

I am somewhat confused. My understanding is that success is counted when the request should respond successfully. The exception is counted when the program throws a unknown exception. Why do you need a realSuccess? The number of success and excption should be independent of each other.

The name successQps might be confusing here. The successQps in Sentinel actually means complete (i.e. passed from Sentinel checking, and has finished invocation, no matter it succeeded or failed logically), so the success count actually includes the exception count. The real success count (what you've understood) should be success - exception.

understood, the latest code had commit.

sczyh30 · 2019-06-13T12:37:24Z

...emo-basic/src/main/java/com/alibaba/csp/sentinel/demo/degrade/ExceptionRatioDegradeDemo.java

@@ -113,8 +113,9 @@ private static void initDegradeRule() {
        rule.setResource(KEY);
        // set limit exception ratio to 0.1
        rule.setCount(0.1);
-        rule.setGrade(RuleConstant.DEGRADE_GRADE_EXCEPTION_RATIO);
+        rule.setGrade(RuleConstant.DEGRADE_GRADE_EXCEPTION_COUNT);


Should be DEGRADE_GRADE_EXCEPTION_RATIO?

sczyh30

LGTM

sczyh30 · 2019-06-14T09:42:37Z

Nice work, Thanks for contributing! 👍
I'll refine the naming of some constants later.

…butes - improvement of #789 Signed-off-by: Eric Zhao <sczyh16@gmail.com>

Lin.Liang and others added 6 commits May 9, 2019 14:54

com.alibaba.csp.sentinel.node.StatisticNode#curThreadNum

11dba5e

using the LongAdder rather than AtomicInteger to Provides better performance

reformat the code according to Alibaba Java Coding Guideline

362f326

reformat the code according to Alibaba Java Coding Guideline

50d2a5a

Fix issue 673: make the value RT_MAX_EXCEED_N in DegradeRule configur…

10db106

…able

Fix issue 673: make the value RT_MAX_EXCEED_N in DegradeRule configur…

cb3dc13

…able make some test

Merge pull request #1 from alibaba/master

509ecf2

merge master

Merge remote-tracking branch 'remotes/origin/master' into 20190524-is…

27a7f77

…sue-673

sczyh30 added the to-review To review label May 25, 2019

sczyh30 reviewed May 27, 2019

View reviewed changes

liangl added 3 commits May 28, 2019 16:28

Remove the one redundant space....

01bda6b

Merge branch '20190528-reformat-code' into 20190524-issue-673

abc8615

Fix issue 673: using rtSlowRequestAmount and minRequestAmount to repl…

cca0e12

…ace rtMaxExceedN

Fix issue 673: Completing test cases and improving code

11cbc7f

sczyh30 requested changes Jun 12, 2019

View reviewed changes

Fix issue 673: fix code problem and improve test case

f8fb243

sczyh30 reviewed Jun 13, 2019

View reviewed changes

Fix issue 673: fix initDegradeRule() method error

51852ee

sczyh30 approved these changes Jun 14, 2019

View reviewed changes

sczyh30 merged commit 2eecd3a into alibaba:master Jun 14, 2019

sczyh30 removed the to-review To review label Jun 14, 2019

sczyh30 mentioned this pull request Jun 14, 2019

Make the value of RT_MAX_EXCEED_N in DegradeRule configurable #673

Closed

sczyh30 added a commit that referenced this pull request Jun 17, 2019

Refine the DegradeRule and add validating logic for the two new attri…

00f116e

…butes - improvement of #789 Signed-off-by: Eric Zhao <sczyh16@gmail.com>

sentinel-bot mentioned this pull request Jun 21, 2019

[WeeklyReport] Weekly report for Sentinel 2019-6-14 to 2019-6-21 #852

Closed

pouchrobot mentioned this pull request Jun 21, 2019

WeeklyReport of Sentinel from 2019-06-14 to 2019-06-21 #853

Closed

sczyh30 mentioned this pull request Jun 27, 2019

Enhancement for circuit breaking #790

Closed

sczyh30 added this to the 1.7.0 milestone Nov 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 673 Make the value of RT_MAX_EXCEED_N in DegradeRule configurable #789

Fix issue 673 Make the value of RT_MAX_EXCEED_N in DegradeRule configurable #789

linlinisme commented May 25, 2019

codecov-io commented May 25, 2019 •

edited

Loading

sczyh30 May 27, 2019

linlinisme May 27, 2019

sczyh30 May 29, 2019

sczyh30 Jun 5, 2019

linlinisme Jun 5, 2019

sczyh30 commented Jun 5, 2019

linlinisme commented Jun 6, 2019 •

edited

Loading

sczyh30 commented Jun 10, 2019

linlinisme commented Jun 11, 2019

sczyh30 Jun 12, 2019

linlinisme Jun 13, 2019

sczyh30 Jun 13, 2019

linlinisme Jun 13, 2019

sczyh30 Jun 13, 2019

sczyh30 left a comment

sczyh30 commented Jun 14, 2019

Fix issue 673 Make the value of RT_MAX_EXCEED_N in DegradeRule configurable #789

Fix issue 673 Make the value of RT_MAX_EXCEED_N in DegradeRule configurable #789

Conversation

linlinisme commented May 25, 2019

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

codecov-io commented May 25, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sczyh30 commented Jun 5, 2019

linlinisme commented Jun 6, 2019 • edited Loading

sczyh30 commented Jun 10, 2019

linlinisme commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sczyh30 left a comment

Choose a reason for hiding this comment

sczyh30 commented Jun 14, 2019

codecov-io commented May 25, 2019 •

edited

Loading

linlinisme commented Jun 6, 2019 •

edited

Loading