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

[feature] integrate datasource of OpenSergo by OpenSergo Go SDK #489

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jnan806
Copy link
Contributor

@jnan806 jnan806 commented Oct 14, 2022

Does this pull request fix one issue?

fixed #483

Special notes for reviews

### Prepare Environment for Test Version

- Prepare the OpenSergo GO SDK.
Because of the OpenSergo GO SDK has no published version, so should download the sourcecode of OpenSergo GO SDKjnan806/opensergo-go-sdk/tree/initial-version, and move it into you GOPATH.

- Rename the right version in directory name of OpenSergo GO SDK sourcecode.
Make sure the version in sourcecode directory name is the same with go.mod.
eg. $GOPATH/pkg/mod/github.com/opensergo/opensergo-go@v0.0.0-20220331070310-e5b01fee4d1c

The jnan806/opensergo-go-sdk/tree/initial-version has been merged into opensergo/opensergo-go-sdk.
So this step of Prepare the OpenSergo GO SDK` is unnecessary.
It's only need to add use sentinel-go with the opensergo-datasource.

Samples

@sczyh30 sczyh30 added kind/feature Category issues or PRs related to feature request to-review PRs to review area/integrations Issue related to integrations with open-source components area/data-source Issues or PRs related to data-source extension labels Oct 14, 2022
@jnan806 jnan806 force-pushed the fix#483-datasource-opensergo branch 2 times, most recently from 217aa0c to 9990e57 Compare October 17, 2022 05:49
@jnan806 jnan806 force-pushed the fix#483-datasource-opensergo branch 4 times, most recently from 176b9ad to 7bcc31e Compare October 18, 2022 00:55
@jnan806 jnan806 marked this pull request as draft October 18, 2022 13:39
pkg/datasource/opensergo/opensergo.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/mixed_property_adaptor.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/mixed_property_adaptor.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/mixed_property_adaptor.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/mixed_property_adaptor.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/mixed_property_adaptor.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo.go Outdated Show resolved Hide resolved
Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

And please update with the latest Go SDK commit (with the new model).

@sczyh30
Copy link
Member

sczyh30 commented Dec 3, 2022

@binbin0325 @louyuting @luckyxiaoqiang Any further suggestions?

@sczyh30
Copy link
Member

sczyh30 commented Dec 26, 2022

We might need to discuss the model of the OpenSergo data-source soon (to make it clear).

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

  1. 数据源实现看一下是不是可以完全实现 Sentinel DataSource 接口,做到标准化;
  2. 去除目前数据源内部的增量变更逻辑,后续可以和 Sentinel Go 社区一起来看这个设计;
  3. 订阅/取消订阅相关的 API 遵循 Sentinel 的语义;
  4. 可以添加一下熔断规则的支持,看看目前的模型是否满足 多种规则转换,及多种 strategy 转到同种 Sentinel 规则的需要。
  5. update apply 时做好更新范围管理

pkg/datasource/opensergo/opensergo.go Show resolved Hide resolved
jnan806 and others added 3 commits January 5, 2023 09:30
…oDataSouce

Signed-off-by: Jiangnan Jia <jnan0806@gmail.com>
Signed-off-by: Jiangnan Jia <jnan0806@gmail.com>
…fd77bd6ce

Signed-off-by: Jiangnan Jia <jnan0806@gmail.com>
@jnan806 jnan806 force-pushed the fix#483-datasource-opensergo branch 3 times, most recently from df4a242 to f28ec80 Compare January 5, 2023 08:02
@sczyh30
Copy link
Member

sczyh30 commented Jan 9, 2023

cc @binbin0325 for update

@jnan806 jnan806 force-pushed the fix#483-datasource-opensergo branch from f28ec80 to 20320de Compare January 10, 2023 01:40
Signed-off-by: Jiangnan Jia <jnan0806@gmail.com>
@jnan806 jnan806 force-pushed the fix#483-datasource-opensergo branch from 20320de to 1d99c71 Compare January 11, 2023 01:01
@jnan806
Copy link
Contributor Author

jnan806 commented Jan 11, 2023

@sczyh30

Cloud you help me to check and complete the // TODO list in file fix#483-datasource-opensergo -> opensergo_sentinel_assembler.go to fill filed-mapping between pb-message and sentinel rule.
And then delete the // TODO comment line.

You can commit into the branch jnan806/sentinel-golang/fix#483-datasource-opensergo directly.

pkg/datasource/opensergo/opensergo.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo_rule_aggregator.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo_const.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo_rule_aggregator.go Outdated Show resolved Hide resolved
pkg/datasource/opensergo/opensergo_rule_aggregator.go Outdated Show resolved Hide resolved
func (ds *OpenSergoDataSource) ReadSource() ([]byte, error) {
// assemble updated MixedRule
mixedRule := new(MixedRule)
if ds.opensergoRuleAggregator.mixedRuleCache.updateFlagMap[RuleType_FlowRule] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

感觉这个mixedRuleCache 可以去掉, 在doUpdate func 中接收更新的RuleType参数,在ReadSource根据更新的RuleType 参数设置到mixedRule即可。

Copy link
Contributor Author

@jnan806 jnan806 Jan 11, 2023

Choose a reason for hiding this comment

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

我是这么想的,mixedRule 就仅作为 rule 的一个集合,不带业务属性比较好。用另外的mixedRuleCache这个结构体来包含 更新标志,这样结构体的功能区分会更加清晰。

Signed-off-by: Jiangnan Jia <jnan0806@gmail.com>
Comment on lines +76 to +81
// TODO fill field-mapping between sentinel-rule and pb-message
flowRule.Threshold = float64(pbStrategy.Threshold)
flowRule.TokenCalculateStrategy = flow.Direct
flowRule.ControlBehavior = flow.Reject

return flowRule
Copy link
Contributor Author

@jnan806 jnan806 Jan 11, 2023

Choose a reason for hiding this comment

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

@binbin0325
Cloud you help me to check and complete the field-mapping
And then delete the // TODO comment line.

You can commit into the branch jnan806/sentinel-golang/fix#483-datasource-opensergo directly.
I have already invite you into this repo.

Comment on lines +130 to +136
// TODO fill field-mapping between sentinel-rule and pb-message
flowRule.Threshold = float64(1000 / pbStrategy.MinIntervalMillisOfRequests)
flowRule.TokenCalculateStrategy = flow.Direct
flowRule.ControlBehavior = flow.Throttling
flowRule.MaxQueueingTimeMs = uint32(pbStrategy.QueueTimeoutMillis)

return flowRule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@binbin0325 the same as above

Comment on lines +185 to +189
// TODO fill field-mapping between sentinel-rule and pb-message
isolationRule.Threshold = uint32(pbStrategy.MaxConcurrency)
isolationRule.MetricType = isolation.Concurrency

return isolationRule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@binbin0325 the same as above

Comment on lines +238 to +251
// TODO fill field-mapping between sentinel-rule and pb-message
switch pbStrategy.Strategy {
case faulttolerancePb.CircuitBreakerStrategy_STRATEGY_SLOW_REQUEST_RATIO:
circuitbreakerRule.Threshold = float64(pbStrategy.SlowCondition.MaxAllowedRtMillis)
break
case faulttolerancePb.CircuitBreakerStrategy_STRATEGY_ERROR_REQUEST_RATIO:
circuitbreakerRule.Threshold = pbStrategy.TriggerRatio
break
default:
logging.Info("[OpenSergoDatasource] unknow CircuitBreakerStrategy.", "resourceName", circuitbreakerRule.Resource, "pbStrategy", pbStrategy)
}
circuitbreakerRule.MinRequestAmount = uint64(pbStrategy.MinRequestAmount)

return circuitbreakerRule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@binbin0325 the same as above

logging.Info("[OpenSergoDatasource] un-subscribing OpenSergo ConcurrencyLimitStrategy.", "namespace", ds.namespace, "app", ds.app)
}

func (ds *OpenSergoDataSource) subscribeCircuitBreakerRule() {
Copy link
Member

Choose a reason for hiding this comment

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

These subscribe/unsubscribe methods for Sentinel rules should be public.

Copy link
Member

Choose a reason for hiding this comment

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

And please pay attention to error handling. Do not ignore the error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These subscribe/unsubscribe methods for Sentinel rules should be public.

i make it private because i have no idea about how to avoid re-invoking this functions.
The subscribe functions have been invoked in Initialize(). If make them public , users would invoke twice, also unsubscribe twice will be invoked twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/data-source Issues or PRs related to data-source extension area/integrations Issue related to integrations with open-source components kind/feature Category issues or PRs related to feature request to-review PRs to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add data-source extension for OpenSergo rate limiting and fault-tolerance spec
3 participants