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

Refactor the underlying model of SystemRuleManager #3158

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

Conversation

huang-gz
Copy link

@huang-gz huang-gz commented Jun 29, 2023

Describe what this PR does / why we need it

重构 SystemRule, 参考了 sentinel-golang 对 SystemRule 的实现, 一个系统规则对应一个指标条件

Does this pull request fix one issue?

Fixes #3143 #3084 #3148

Describe how you did it

  1. 一个系统规则对应一个条件, 系统规则包含类型和触发值 (SystemMetricType 和 triggerCount)
  2. 简化 SystemRuleManager 职责, 只负责进行规则的校验和加载
  3. 增加 SystemRuleChecker 负责根据规则去判断当前请求是否触发阈值

Describe how to verify it

运行单元测试

  1. com.alibaba.csp.sentinel.slots.block.system.SystemRuleTest
  2. com.alibaba.csp.sentinel.slots.system.SystemRuleManagerTest
    运行验证 demo
    com.alibaba.csp.sentinel.demo.system.SystemGuardDemo#main()

Special notes for reviews

@huang-gz huang-gz changed the title Refactor the underlying model of SystemRuleManager 1:1 Refactor the underlying model of SystemRuleManager Jun 29, 2023
@sczyh30 sczyh30 added to-review To review kind/refactor Issue related to functional refactoring. labels Jul 3, 2023
assertEquals(1.2d, rules.get(0).getHighestSystemLoad(), 0.01);
assertEquals(1.2d, SystemRuleManager.getSystemLoadThreshold(), 0.01);
assertEquals(1.2d, rules.get(0).getTriggerCount(), 0.01);
assertEquals(1.2d, SystemRuleManager.getRules().get(0).getTriggerCount(), 0.01);
}

@Test
public void testCheckMaxCpuUsageNotBBR() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test failed, please fix it.

public int getType() {
return type;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

A formatting mistake here.

public void setMaxThread(long maxThread) {
this.maxThread = maxThread;
}
private double triggerCount;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just use count to name it like any other rule.

*/
public class SystemRuleManagerTest {

@Test
public void testLoadInvalidRules() {
SystemRule rule1 = new SystemRule();
rule1.setHighestSystemLoad(-0.9d);
rule1.setTriggerCount(-0.9d);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to avoid this naming scheme, loadRule, cpuUsageRule may be more fitting.

@@ -87,19 +87,33 @@ public void run() {
}

private static void initSystemRule() {
SystemRule rule = new SystemRule();
SystemRule rule1 = new SystemRule();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

@@ -0,0 +1,41 @@
package com.alibaba.csp.sentinel.slots.system;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please prepend the license header here?

@@ -0,0 +1,116 @@
package com.alibaba.csp.sentinel.slots.system;
Copy link
Collaborator

@LearningGp LearningGp Dec 24, 2023

Choose a reason for hiding this comment

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

Ditto.

@LearningGp LearningGp added the inactive Category issues or prs not active. label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Category issues or prs not active. kind/refactor Issue related to functional refactoring. to-review To review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants