You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2020/07/18 16:17:48 UTC
[GitHub] [dubbo-go] williamfeng323 opened a new pull request #662: [WIP]Ftr: Feature/application level router
williamfeng323 opened a new pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662
<!-- Thanks for sending a pull request!
-->
**What this PR does**:
1. Support multiple routers configuration in router_config.yml
2. Apply prioritization to routers
3. Enable application-level router
**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #596
**Special notes for your reviewer**:
**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] williamfeng323 commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
williamfeng323 commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r467072831
##########
File path: cluster/directory/base_directory.go
##########
@@ -104,6 +110,24 @@ func (dir *BaseDirectory) SetRouters(urls []*common.URL) {
rc.AddRouters(routers)
}
+func (dir *BaseDirectory) isProperRouter(url *common.URL) bool {
+ app := url.GetParam(constant.APPLICATION_KEY, "")
+ serviceKey := dir.GetUrl().ServiceKey()
+ if serviceKey == "" {
+ serviceKey = dir.GetUrl().SubURL.ServiceKey()
+ }
+ if len(app) > 0 {
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] codecov-commenter commented on pull request #662: [WIP]Ftr: Feature/application level router
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#issuecomment-660615640
# [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=h1) Report
> Merging [#662](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/dubbo-go/commit/d3db1c2ef966b5a3092ddfbb9fa5603f1fd92aad&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `72.72%`.
[](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## develop #662 +/- ##
===========================================
+ Coverage 63.80% 63.81% +0.01%
===========================================
Files 236 236
Lines 12313 12329 +16
===========================================
+ Hits 7856 7868 +12
Misses 3690 3690
- Partials 767 771 +4
```
| [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cluster/router/tag/router\_rule.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL3JvdXRlcl9ydWxlLmdv) | `71.42% <ø> (ø)` | |
| [common/yaml/yaml.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y29tbW9uL3lhbWwveWFtbC5nbw==) | `86.66% <0.00%> (-13.34%)` | :arrow_down: |
| [config/consumer\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y29uZmlnL2NvbnN1bWVyX2NvbmZpZy5nbw==) | `56.25% <ø> (ø)` | |
| [config/provider\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y29uZmlnL3Byb3ZpZGVyX2NvbmZpZy5nbw==) | `58.06% <ø> (ø)` | |
| [config/router\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y29uZmlnL3JvdXRlcl9jb25maWcuZ28=) | `70.37% <73.33%> (-4.63%)` | :arrow_down: |
| [cluster/router/condition/file.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvY29uZGl0aW9uL2ZpbGUuZ28=) | `91.30% <100.00%> (+0.60%)` | :arrow_up: |
| [cluster/router/condition/router\_rule.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvY29uZGl0aW9uL3JvdXRlcl9ydWxlLmdv) | `84.61% <100.00%> (ø)` | |
| [cluster/cluster\_impl/base\_cluster\_invoker.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvYmFzZV9jbHVzdGVyX2ludm9rZXIuZ28=) | `61.11% <0.00%> (-9.73%)` | :arrow_down: |
| [cluster/cluster\_impl/failback\_cluster\_invoker.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvZmFpbGJhY2tfY2x1c3Rlcl9pbnZva2VyLmdv) | `78.02% <0.00%> (-2.20%)` | :arrow_down: |
| [protocol/dubbo/client.go](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree#diff-cHJvdG9jb2wvZHViYm8vY2xpZW50Lmdv) | `67.87% <0.00%> (-1.22%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/dubbo-go/pull/662/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=footer). Last update [c6c39e2...b501734](https://codecov.io/gh/apache/dubbo-go/pull/662?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] zouyx commented on a change in pull request #662: [WIP]Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
zouyx commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r461300280
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,41 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ } else {
+ grp, srv, ver, _ := parseServiceRouterKey(routerRule.Key)
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
+ }
})
return f.url
}
+func parseServiceRouterKey(key string) (string, string, string, error) {
+ if len(strings.TrimSpace(key)) == 0 {
+ return "", "", "", nil
+ }
+ reg := regexp.MustCompile(`(.*/{1})?([^:/]+)(:{1}[^:]*)?`)
+ strs := reg.FindAllStringSubmatch(key, -1)
+ if strs == nil || len(strs) != 1 {
Review comment:
```suggestion
if strs == nil || len(strs) > 1 {
```
you mean greater than 1 or greater than 0?
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,41 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ } else {
+ grp, srv, ver, _ := parseServiceRouterKey(routerRule.Key)
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
+ }
})
return f.url
}
+func parseServiceRouterKey(key string) (string, string, string, error) {
+ if len(strings.TrimSpace(key)) == 0 {
+ return "", "", "", nil
+ }
+ reg := regexp.MustCompile(`(.*/{1})?([^:/]+)(:{1}[^:]*)?`)
+ strs := reg.FindAllStringSubmatch(key, -1)
+ if strs == nil || len(strs) != 1 {
+ return "", "", "", perrors.Errorf("Invalid key, service key must follow [{group}/]{service}[:{version}] pattern")
+ }
+ grp := strings.TrimSpace(strings.TrimRight(strs[0][1], "/"))
+ srv := strings.TrimSpace(strs[0][2])
+ ver := strings.TrimSpace(strings.TrimLeft(strs[0][3], ":"))
Review comment:
Should you check `len(strs[0]) ==3 ` or `len(strs[0]) >2 `
##########
File path: cluster/router/condition/router_rule.go
##########
@@ -57,7 +58,7 @@ func getRule(rawRule string) (*RouterRule, error) {
return r, err
}
r.RawRule = rawRule
- if len(r.Conditions) != 0 {
+ if len(r.Conditions) != 0 && r.Key != "" && (r.Scope == constant.RouterApplicationScope || r.Scope == constant.RouterServiceScope) {
Review comment:
```suggestion
if len(r.Conditions) > 0 && len(r.Key)>0 && (r.Scope == constant.RouterApplicationScope || r.Scope == constant.RouterServiceScope) {
```
##########
File path: cluster/router/condition/router.go
##########
@@ -117,7 +117,13 @@ func NewConditionRouter(url *common.URL) (*ConditionRouter, error) {
}
router.url = url
- router.priority = url.GetParamInt(constant.RouterPriority, 0)
+ var defaultPriority int64
+ if url.GetParam(constant.APPLICATION_KEY, "") != "" {
+ defaultPriority = 150
+ } else {
+ defaultPriority = 140
Review comment:
why change default priotiry ?
##########
File path: config/router_config.go
##########
@@ -32,16 +33,37 @@ var (
routerURLSet = gxset.NewSet()
)
+// LocalRouterRules defines the local router config structure
+type LocalRouterRules struct {
+ RouterRules []interface{} `yaml:"routerRules"`
+}
+
// RouterInit Load config file to init router config
func RouterInit(confRouterFile string) error {
- fileRouterFactories := extension.GetFileRouterFactories()
bytes, err := yaml.LoadYMLConfig(confRouterFile)
if err != nil {
return perrors.Errorf("ioutil.ReadFile(file:%s) = error:%v", confRouterFile, perrors.WithStack(err))
}
- logger.Warnf("get fileRouterFactories len{%+v})", len(fileRouterFactories))
- for k, factory := range fileRouterFactories {
- r, e := factory.NewFileRouter(bytes)
+ routerRules := &LocalRouterRules{}
+ err = yaml.UnmarshalYML(bytes, routerRules)
+ if err != nil {
+ return perrors.Errorf("Load router file %s failed due to error: %v", confRouterFile, perrors.WithStack(err))
+ }
+ if len(routerRules.RouterRules) == 0 {
+ return perrors.Errorf("No router configurations in file %s", confRouterFile)
+ }
+ fileRouterFactories := extension.GetFileRouterFactories()
+ for _, v := range routerRules.RouterRules {
+ content, _ := yaml.MarshalYML(v)
+ err = initRouterConfig(content, fileRouterFactories)
+ }
+ return err
Review comment:
i think should add a interface here, for load the router config file.
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,41 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ } else {
+ grp, srv, ver, _ := parseServiceRouterKey(routerRule.Key)
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
+ }
Review comment:
```suggestion
if routerRule.Scope == constant.RouterApplicationScope {
f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
return
}
grp, srv, ver, e := parseServiceRouterKey(routerRule.Key)
if e != nil {
return
}
if len(grp) > 0 {
f.url.AddParam(constant.GROUP_KEY, grp)
}
if len(ver) > 0 {
f.url.AddParam(constant.VERSION_KEY, ver)
}
if len(srv) > 0 {
f.url.AddParam(constant.INTERFACE_KEY, srv)
}
```
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,41 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ } else {
+ grp, srv, ver, _ := parseServiceRouterKey(routerRule.Key)
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
+ }
})
return f.url
}
+func parseServiceRouterKey(key string) (string, string, string, error) {
+ if len(strings.TrimSpace(key)) == 0 {
+ return "", "", "", nil
+ }
+ reg := regexp.MustCompile(`(.*/{1})?([^:/]+)(:{1}[^:]*)?`)
+ strs := reg.FindAllStringSubmatch(key, -1)
+ if strs == nil || len(strs) != 1 {
+ return "", "", "", perrors.Errorf("Invalid key, service key must follow [{group}/]{service}[:{version}] pattern")
+ }
+ grp := strings.TrimSpace(strings.TrimRight(strs[0][1], "/"))
+ srv := strings.TrimSpace(strs[0][2])
+ ver := strings.TrimSpace(strings.TrimLeft(strs[0][3], ":"))
Review comment:
Should you check `len(strs[0]) ==4 ` or `len(strs[0]) >3 `
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] williamfeng323 commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
williamfeng323 commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r466904163
##########
File path: config/router_config.go
##########
@@ -32,16 +33,37 @@ var (
routerURLSet = gxset.NewSet()
)
+// LocalRouterRules defines the local router config structure
+type LocalRouterRules struct {
+ RouterRules []interface{} `yaml:"routerRules"`
+}
+
// RouterInit Load config file to init router config
func RouterInit(confRouterFile string) error {
- fileRouterFactories := extension.GetFileRouterFactories()
bytes, err := yaml.LoadYMLConfig(confRouterFile)
if err != nil {
return perrors.Errorf("ioutil.ReadFile(file:%s) = error:%v", confRouterFile, perrors.WithStack(err))
}
- logger.Warnf("get fileRouterFactories len{%+v})", len(fileRouterFactories))
- for k, factory := range fileRouterFactories {
- r, e := factory.NewFileRouter(bytes)
+ routerRules := &LocalRouterRules{}
+ err = yaml.UnmarshalYML(bytes, routerRules)
+ if err != nil {
+ return perrors.Errorf("Load router file %s failed due to error: %v", confRouterFile, perrors.WithStack(err))
+ }
+ if len(routerRules.RouterRules) == 0 {
+ return perrors.Errorf("No router configurations in file %s", confRouterFile)
+ }
+ fileRouterFactories := extension.GetFileRouterFactories()
+ for _, v := range routerRules.RouterRules {
+ content, _ := yaml.MarshalYML(v)
+ err = initRouterConfig(content, fileRouterFactories)
+ }
+ return err
Review comment:
as discussed, this will be checked in the factory, so do not create the interface for now. We may refactor this part in the future version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] pantianying commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
pantianying commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r467105300
##########
File path: cluster/directory/base_directory.go
##########
@@ -104,6 +110,21 @@ func (dir *BaseDirectory) SetRouters(urls []*common.URL) {
rc.AddRouters(routers)
}
+func (dir *BaseDirectory) isProperRouter(url *common.URL) bool {
+ app := url.GetParam(constant.APPLICATION_KEY, "")
+ serviceKey := dir.GetUrl().ServiceKey()
+ if serviceKey == "" {
+ serviceKey = dir.GetUrl().SubURL.ServiceKey()
+ }
+ if len(app) > 0 && app != dir.GetUrl().GetParam(constant.APPLICATION_KEY, "") {
+ return false
+ }
+ if url.ServiceKey() != serviceKey {
+ return false
+ }
+ return true
+}
+
Review comment:
This means that when app = = “”, it must return true
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] williamfeng323 commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
williamfeng323 commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r467408323
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,47 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ return
+ }
+ grp, srv, ver, e := parseServiceRouterKey(routerRule.Key)
+ if e != nil {
+ return
+ }
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
})
return f.url
}
+func parseServiceRouterKey(key string) (string, string, string, error) {
Review comment:
added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] williamfeng323 commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
williamfeng323 commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r467405633
##########
File path: cluster/directory/base_directory.go
##########
@@ -104,6 +110,21 @@ func (dir *BaseDirectory) SetRouters(urls []*common.URL) {
rc.AddRouters(routers)
}
+func (dir *BaseDirectory) isProperRouter(url *common.URL) bool {
+ app := url.GetParam(constant.APPLICATION_KEY, "")
+ serviceKey := dir.GetUrl().ServiceKey()
+ if serviceKey == "" {
+ serviceKey = dir.GetUrl().SubURL.ServiceKey()
+ }
+ if len(app) > 0 && app != dir.GetUrl().GetParam(constant.APPLICATION_KEY, "") {
+ return false
+ }
+ if url.ServiceKey() != serviceKey {
+ return false
+ }
+ return true
+}
+
Review comment:
updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] williamfeng323 commented on a change in pull request #662: [WIP]Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
williamfeng323 commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r461307297
##########
File path: cluster/router/condition/router.go
##########
@@ -117,7 +117,13 @@ func NewConditionRouter(url *common.URL) (*ConditionRouter, error) {
}
router.url = url
- router.priority = url.GetParamInt(constant.RouterPriority, 0)
+ var defaultPriority int64
+ if url.GetParam(constant.APPLICATION_KEY, "") != "" {
+ defaultPriority = 150
+ } else {
+ defaultPriority = 140
Review comment:
This is the default priority for app router and service router. to ensure the service router has higher priority compare to app router if no priority is set in configuration.
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,41 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ } else {
+ grp, srv, ver, _ := parseServiceRouterKey(routerRule.Key)
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
+ }
})
return f.url
}
+func parseServiceRouterKey(key string) (string, string, string, error) {
+ if len(strings.TrimSpace(key)) == 0 {
+ return "", "", "", nil
+ }
+ reg := regexp.MustCompile(`(.*/{1})?([^:/]+)(:{1}[^:]*)?`)
+ strs := reg.FindAllStringSubmatch(key, -1)
+ if strs == nil || len(strs) != 1 {
+ return "", "", "", perrors.Errorf("Invalid key, service key must follow [{group}/]{service}[:{version}] pattern")
+ }
+ grp := strings.TrimSpace(strings.TrimRight(strs[0][1], "/"))
+ srv := strings.TrimSpace(strs[0][2])
+ ver := strings.TrimSpace(strings.TrimLeft(strs[0][3], ":"))
Review comment:
i am gonna check len(strs[0]) != 4 since the regexp expected to return size == 4 slice to represent 1st slice: origin string; 2nd/3rd/4th slice: the first/second/third captured value even it is empty;
##########
File path: config/router_config.go
##########
@@ -32,16 +33,37 @@ var (
routerURLSet = gxset.NewSet()
)
+// LocalRouterRules defines the local router config structure
+type LocalRouterRules struct {
+ RouterRules []interface{} `yaml:"routerRules"`
+}
+
// RouterInit Load config file to init router config
func RouterInit(confRouterFile string) error {
- fileRouterFactories := extension.GetFileRouterFactories()
bytes, err := yaml.LoadYMLConfig(confRouterFile)
if err != nil {
return perrors.Errorf("ioutil.ReadFile(file:%s) = error:%v", confRouterFile, perrors.WithStack(err))
}
- logger.Warnf("get fileRouterFactories len{%+v})", len(fileRouterFactories))
- for k, factory := range fileRouterFactories {
- r, e := factory.NewFileRouter(bytes)
+ routerRules := &LocalRouterRules{}
+ err = yaml.UnmarshalYML(bytes, routerRules)
+ if err != nil {
+ return perrors.Errorf("Load router file %s failed due to error: %v", confRouterFile, perrors.WithStack(err))
+ }
+ if len(routerRules.RouterRules) == 0 {
+ return perrors.Errorf("No router configurations in file %s", confRouterFile)
+ }
+ fileRouterFactories := extension.GetFileRouterFactories()
+ for _, v := range routerRules.RouterRules {
+ content, _ := yaml.MarshalYML(v)
+ err = initRouterConfig(content, fileRouterFactories)
+ }
+ return err
Review comment:
Do you mean loading different types of config files to support the earlier version of router config files?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] pantianying commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
pantianying commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r467105300
##########
File path: cluster/directory/base_directory.go
##########
@@ -104,6 +110,21 @@ func (dir *BaseDirectory) SetRouters(urls []*common.URL) {
rc.AddRouters(routers)
}
+func (dir *BaseDirectory) isProperRouter(url *common.URL) bool {
+ app := url.GetParam(constant.APPLICATION_KEY, "")
+ serviceKey := dir.GetUrl().ServiceKey()
+ if serviceKey == "" {
+ serviceKey = dir.GetUrl().SubURL.ServiceKey()
+ }
+ if len(app) > 0 && app != dir.GetUrl().GetParam(constant.APPLICATION_KEY, "") {
+ return false
+ }
+ if url.ServiceKey() != serviceKey {
+ return false
+ }
+ return true
+}
+
Review comment:
This means that when app = = = ', it must return true
##########
File path: cluster/router/condition/file.go
##########
@@ -71,11 +72,47 @@ func (f *FileConditionRouter) URL() common.URL {
common.WithParamsValue(constant.RouterPriority, strconv.Itoa(routerRule.Priority)),
common.WithParamsValue(constant.RULE_KEY, base64.URLEncoding.EncodeToString([]byte(rule))),
common.WithParamsValue(constant.ROUTER_KEY, constant.CONDITION_ROUTE_PROTOCOL),
- common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY))
+ common.WithParamsValue(constant.CATEGORY_KEY, constant.ROUTERS_CATEGORY),
+ )
+ if routerRule.Scope == constant.RouterApplicationScope {
+ f.url.AddParam(constant.APPLICATION_KEY, routerRule.Key)
+ return
+ }
+ grp, srv, ver, e := parseServiceRouterKey(routerRule.Key)
+ if e != nil {
+ return
+ }
+ if len(grp) > 0 {
+ f.url.AddParam(constant.GROUP_KEY, grp)
+ }
+ if len(ver) > 0 {
+ f.url.AddParam(constant.VERSION_KEY, ver)
+ }
+ if len(srv) > 0 {
+ f.url.AddParam(constant.INTERFACE_KEY, srv)
+ }
})
return f.url
}
+func parseServiceRouterKey(key string) (string, string, string, error) {
Review comment:
please add some Examples as comments
##########
File path: cluster/router/condition/router_test.go
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package condition
+
+import (
+ "testing"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+)
+
+import (
+ "github.com/dubbogo/gost/container/set"
Review comment:
still have problem？
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] zouyx commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
zouyx commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r464079647
##########
File path: config/router_config.go
##########
@@ -32,16 +33,37 @@ var (
routerURLSet = gxset.NewSet()
)
+// LocalRouterRules defines the local router config structure
+type LocalRouterRules struct {
+ RouterRules []interface{} `yaml:"routerRules"`
+}
+
// RouterInit Load config file to init router config
func RouterInit(confRouterFile string) error {
- fileRouterFactories := extension.GetFileRouterFactories()
bytes, err := yaml.LoadYMLConfig(confRouterFile)
if err != nil {
return perrors.Errorf("ioutil.ReadFile(file:%s) = error:%v", confRouterFile, perrors.WithStack(err))
}
- logger.Warnf("get fileRouterFactories len{%+v})", len(fileRouterFactories))
- for k, factory := range fileRouterFactories {
- r, e := factory.NewFileRouter(bytes)
+ routerRules := &LocalRouterRules{}
+ err = yaml.UnmarshalYML(bytes, routerRules)
+ if err != nil {
+ return perrors.Errorf("Load router file %s failed due to error: %v", confRouterFile, perrors.WithStack(err))
+ }
+ if len(routerRules.RouterRules) == 0 {
+ return perrors.Errorf("No router configurations in file %s", confRouterFile)
+ }
+ fileRouterFactories := extension.GetFileRouterFactories()
+ for _, v := range routerRules.RouterRules {
+ content, _ := yaml.MarshalYML(v)
+ err = initRouterConfig(content, fileRouterFactories)
+ }
+ return err
Review comment:
I just mean loading different types of config files to support all router config files in `dubbo-go`.
After all, there are three types of router in dubbo-go at least .
* condition
* tag
* health instance first
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] williamfeng323 commented on a change in pull request #662: [WIP]Ftr: Feature/application level router
Posted by GitBox <gi...@apache.org>.
williamfeng323 commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r456808377
##########
File path: config/router_config_test.go
##########
@@ -28,9 +28,11 @@ import (
import (
_ "github.com/apache/dubbo-go/cluster/router/condition"
+ "github.com/dubbogo/gost/container/set"
Review comment:
resolved
##########
File path: cluster/router/condition/router_test.go
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package condition
+
+import (
+ "testing"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+)
+
+import (
+ "github.com/dubbogo/gost/container/set"
Review comment:
resolved
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #662: [WIP]Ftr: Feature/application level router
Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r456805716
##########
File path: cluster/router/condition/router_test.go
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package condition
+
+import (
+ "testing"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+)
+
+import (
+ "github.com/dubbogo/gost/container/set"
Review comment:
this package should be placed in the 2rd import block.
##########
File path: config/router_config_test.go
##########
@@ -28,9 +28,11 @@ import (
import (
_ "github.com/apache/dubbo-go/cluster/router/condition"
+ "github.com/dubbogo/gost/container/set"
Review comment:
it should be placed in the 2rd import block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] zouyx commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
zouyx commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r464079118
##########
File path: cluster/router/condition/router.go
##########
@@ -117,7 +117,13 @@ func NewConditionRouter(url *common.URL) (*ConditionRouter, error) {
}
router.url = url
- router.priority = url.GetParamInt(constant.RouterPriority, 0)
+ var defaultPriority int64
+ if url.GetParam(constant.APPLICATION_KEY, "") != "" {
+ defaultPriority = 150
+ } else {
+ defaultPriority = 140
Review comment:
No, bro.....
You have got my point.
Look at the left block.....default priority is `0`, you change to `140`.
Your logic is if application router set priority to `150 , if not, set priority to `140`. But original logic is if not, set priority to `0`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] pantianying merged pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
pantianying merged pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #662: Ftr: Feature/application and service level router
Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #662:
URL: https://github.com/apache/dubbo-go/pull/662#discussion_r465647557
##########
File path: cluster/directory/base_directory.go
##########
@@ -104,6 +110,24 @@ func (dir *BaseDirectory) SetRouters(urls []*common.URL) {
rc.AddRouters(routers)
}
+func (dir *BaseDirectory) isProperRouter(url *common.URL) bool {
+ app := url.GetParam(constant.APPLICATION_KEY, "")
+ serviceKey := dir.GetUrl().ServiceKey()
+ if serviceKey == "" {
+ serviceKey = dir.GetUrl().SubURL.ServiceKey()
+ }
+ if len(app) > 0 {
Review comment:
the more clear codes is as follows
if len(app) > 0 && app != dir.GetUrl().GetParam(constant.APPLICATION_KEY, "") {
return false
}
if url.ServiceKey() != serviceKey {
return false
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org