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

feat: Add support for sentinel opensergo datasource #3142

Open
wants to merge 5 commits into
base: 2.2.x-ospp2023
Choose a base branch
from

Conversation

123liuziming
Copy link
Collaborator

@123liuziming 123liuziming commented Feb 6, 2023

Describe what this PR does / why we need it

Add out-of-box support for Sentinel OpenSergo data-source.

If you want to use OpenSergo datasource, you can configure the application.yml like NacosDatasource in Spring Cloud Alibaba:

# you can set single rule type by setting enabled-rules, like the other datasource in Spring Cloud Alibaba
spring.cloud.sentinel.datasource.ds5.opensergo.rule-type=flow
spring.cloud.sentinel.datasource.ds5.opensergo.host=127.0.0.1
spring.cloud.sentinel.datasource.ds5.opensergo.port=10246
spring.cloud.sentinel.datasource.ds5.opensergo.namespace=ns1
spring.cloud.sentinel.datasource.ds5.opensergo.app=app1
# you can set multiple rule type by setting enabled-rules
spring.cloud.sentinel.datasource.ds5.opensergo.enabled-rules[0]=flow
spring.cloud.sentinel.datasource.ds5.opensergo.enabled-rules[0]=degrade

Does this pull request fix one issue?

#3075

@@ -14,6 +14,10 @@
<artifactId>spring-cloud-alibaba-sentinel-datasource</artifactId>
<name>Spring Cloud Alibaba Sentinel DataSource</name>

<properties>
<opensergo-datasource.version>2.0.0-SNAPSHOT</opensergo-datasource.version>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I will fix it


@Override
public void preCheck(String dataSourceName) {
if (StringUtils.isEmpty(app)) {
Copy link
Member

Choose a reason for hiding this comment

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

这里面是否可以简化使用方式:如果 app 没有配,则按照 SCA Sentinel 取 appName 的方式来取?

另外这些 check 是否当且仅当 OpenSergo endpoint 有实际配置时才生效?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 可以
  2. 应该不需要这个preCheck逻辑了,现在所有的属性应该都有默认值

@sczyh30 sczyh30 added area/sentinel spring cloud alibaba sentinel area/OpenSergo Issues or PRs related to OpenSergo labels Feb 6, 2023
Comment on lines +88 to +92
<dependency>
<groupId>com.alibaba.csp</groupId>
<artifactId>sentinel-datasource-opensergo</artifactId>
<version>${opensergo-datasource.version}</version>
</dependency>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whether to use optional, the integration of opensergo is at the discretion of the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this dependency, I can not use the OpenSergoDataSourceGroup

@sczyh30
Copy link
Member

sczyh30 commented Feb 6, 2023

Could you please provide a sample application.properties config for the Sentinel OpenSergo data-source in the PR description?

@123liuziming
Copy link
Collaborator Author

Could you please provide a sample application.properties config for the Sentinel OpenSergo data-source in the PR description?

done


private String app = AppNameUtil.getAppName();

private Set<String> enabledRules = new HashSet<>();
Copy link

@jnan806 jnan806 Apr 6, 2023

Choose a reason for hiding this comment

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

Maybe can use Set<RuleType>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, It is better to use Enum type. I will fix it!

Comment on lines +66 to +74
RuleType ruleType = getRuleType();
switch (ruleType) {
case FLOW:
FlowRuleManager.register2Property(dataSourceGroup.subscribeFlowRules());
break;
case DEGRADE:
DegradeRuleManager.register2Property(dataSourceGroup.subscribeDegradeRules());
break;
}
Copy link

@jnan806 jnan806 Apr 6, 2023

Choose a reason for hiding this comment

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

As your comment, these code lines should be wrapped in condition enabledRules is empty.
Because the ruleType was annotationed by NotNull in AbstractDataSourceProperties ,so if enabledRules contains the ruleType, the subscribeConfig action will be invoked twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/OpenSergo Issues or PRs related to OpenSergo area/sentinel spring cloud alibaba sentinel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants