-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
base: 2.2.x-ospp2023
Are you sure you want to change the base?
feat: Add support for sentinel opensergo datasource #3142
Conversation
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use 0.1.0-beta
version here.
Refer https://opensergo.io/zh-cn/docs/quick-start/java/quick-start-sentinel/
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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 有实际配置时才生效?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 可以
- 应该不需要这个preCheck逻辑了,现在所有的属性应该都有默认值
<dependency> | ||
<groupId>com.alibaba.csp</groupId> | ||
<artifactId>sentinel-datasource-opensergo</artifactId> | ||
<version>${opensergo-datasource.version}</version> | ||
</dependency> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Could you please provide a sample |
done |
|
||
private String app = AppNameUtil.getAppName(); | ||
|
||
private Set<String> enabledRules = new HashSet<>(); |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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!
RuleType ruleType = getRuleType(); | ||
switch (ruleType) { | ||
case FLOW: | ||
FlowRuleManager.register2Property(dataSourceGroup.subscribeFlowRules()); | ||
break; | ||
case DEGRADE: | ||
DegradeRuleManager.register2Property(dataSourceGroup.subscribeDegradeRules()); | ||
break; | ||
} |
There was a problem hiding this comment.
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.
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:Does this pull request fix one issue?
#3075