Ftr: dynamic tag router #665

watermelo · 2020-07-19T14:44:19Z

What this PR does:
#593

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

AlexStocks · 2020-07-30T05:06:26Z

cluster/router/tag/router_rule.go

 	return r, nil
 }

+func (t *RouterRule) init() {
+	t.addressToTagNames = make(map[string][]string)
+	t.tagNameToAddresses = make(map[string][]string)


t.addressToTagNames = make(map[string][]string, 8) t.tagNameToAddresses = make(map[string][]string, 8)

AlexStocks · 2020-07-30T05:07:13Z

cluster/router/tag/router_rule.go

 func (t *RouterRule) getAddresses() []string {
-	// TODO get all tag addresses
-	return nil
+	var result []string


var result = make([]string, 0, 8 * len(t.Tags))

AlexStocks · 2020-07-30T05:07:29Z

cluster/router/tag/router_rule.go

 }

 func (t *RouterRule) getTagNames() []string {
-	// TODO get all tag names
-	return nil
+	var result []string


var result = make([]string, 0, len(t.Tags))

AlexStocks · 2020-07-30T05:08:38Z

cluster/router/tag/tag_router.go

-		// TODO address parse
-		if address == (host + port) {
+		// TODO ip match
+		if address == host+":"+port {


delete "host+":"+port", using net.JoinHostPort(host, port) instead.

cluster/router/tag/router_rule.go

cluster/router/tag/tag.go

cluster/router/tag/tag_router.go

zouyx · 2020-08-02T13:22:33Z

cluster/router/tag/tag_router.go

+		logger.Errorf("Get rule fail, config rule{%s},  error{%v}", rule, err)
+		return
+	}
+	if rule != "" {


cluster/router/tag/tag_router.go

codecov-commenter · 2020-08-03T06:58:26Z

Codecov Report

Merging #665 into develop will decrease coverage by 0.34%.
The diff coverage is 54.91%.

@@             Coverage Diff             @@
##           develop     #665      +/-   ##
===========================================
- Coverage    63.72%   63.37%   -0.35%     
===========================================
  Files          237      239       +2     
  Lines        12295    12598     +303     
===========================================
+ Hits          7835     7984     +149     
- Misses        3706     3829     +123     
- Partials       754      785      +31

Impacted Files	Coverage Δ
cluster/router/condition/listenable_router.go	`54.83% <ø> (ø)`
cluster/router/tag/file.go	`76.92% <ø> (ø)`
cluster/router/tag/tag.go	`0.00% <0.00%> (ø)`
cluster/router/tag/tag_router.go	`54.62% <52.19%> (-15.97%)`	⬇️
cluster/router/tag/router_rule.go	`84.21% <87.09%> (+12.78%)`	⬆️
filter/filter_impl/metrics_filter.go	`86.36% <0.00%> (-13.64%)`	⬇️
registry/kubernetes/listener.go	`65.95% <0.00%> (-6.39%)`	⬇️
filter/filter_impl/hystrix_filter.go	`68.64% <0.00%> (-3.39%)`	⬇️
protocol/dubbo/client.go	`67.87% <0.00%> (-1.22%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 272ddd5...a31a4e2. Read the comment docs.

zouyx

if the comment has resolved you can click resolved button in the comment.

pantianying · 2020-08-05T03:35:57Z

cluster/router/tag/tag_router.go

+	var (
+		result    []protocol.Invoker
+		addresses []string
+	)


The definition can be moved to the top.
And can you break this pile of code into several parts,it is hard for me 😂

pantianying · 2020-08-05T03:37:02Z

cluster/router/tag/tag_router.go

+	}
+
+	pattern = hostAndPort[0]
+	// TODO 常量化


has some easy todo

we should not use Chinese comment in our codes.

pantianying · 2020-08-05T03:38:14Z

cluster/router/tag/tag_router_test.go

@@ -19,32 +19,59 @@ package tag

 import (
 	"context"
+	"fmt"
+	"github.com/stretchr/testify/suite"


pkg import problem

pantianying · 2020-08-05T03:38:50Z

cluster/router/tag/tag_router_test.go

@@ -19,32 +19,59 @@ package tag

 import (
 	"context"
+	"fmt"
+	"github.com/stretchr/testify/suite"


pkg import problem

AlexStocks · 2020-08-05T11:02:14Z

cluster/router/tag/tag_router.go

+	var (
+		result    []protocol.Invoker
+		addresses []string
+	)


AlexStocks · 2020-08-05T11:03:40Z

cluster/router/tag/tag_router.go

+	for _, invoker := range invokers {
+		for _, filter := range filters {
+			if !filter(invoker) {
+				continue OUTER


break OUTER？

AlexStocks · 2020-08-05T11:04:38Z

cluster/router/tag/tag_router.go

+	}
+
+	pattern = hostAndPort[0]
+	// TODO 常量化


we should not use Chinese comment in our codes.

AlexStocks · 2020-08-05T11:05:51Z

cluster/router/tag/tag_router.go

+
+	ipAddress := strings.Split(host, splitCharacter)
+	for i := 0; i < len(mask); i++ {
+		if "*" == mask[i] || mask[i] == ipAddress[i] {


u should add comment for every if/else branch to make it easy to understand.

AlexStocks · 2020-08-05T11:06:16Z

cluster/router/tag/tag_router.go

+
+func getPatternHostAndPort(pattern string, isIpv4 bool) []string {
+	result := make([]string, 2)
+	if strings.HasPrefix(pattern, "[") && strings.Contains(pattern, "]:") {


add some comments for every if/else branch, pls.

hxmhlt · 2020-08-05T14:30:10Z

cluster/router/tag/router_rule.go

 	return r, nil
 }
+
+func (t *RouterRule) init() {


Why create a new func named init? If you create the func ,pls add the comment for it.

hxmhlt · 2020-08-05T14:30:20Z

cluster/router/tag/router_rule.go

+}
+
+func (t *RouterRule) getAddresses() []string {
+	var result = make([]string, 0, 8*len(t.Tags))


Pls add func comment for RouterRule

zouyx · 2020-08-05T15:22:05Z

cluster/router/tag/router_rule.go

+	t.addressToTagNames = make(map[string][]string, 8)
+	t.tagNameToAddresses = make(map[string][]string, 8)


Suggested change

t.addressToTagNames = make(map[string][]string, 8)

t.tagNameToAddresses = make(map[string][]string, 8)

l:=len(t.Tags)

t.addressToTagNames = make(map[string][]string, l*2)

t.tagNameToAddresses = make(map[string][]string, l)

zouyx · 2020-08-05T15:22:59Z

cluster/router/tag/router_rule.go

+}
+
+func (t *RouterRule) getAddresses() []string {
+	var result = make([]string, 0, 8*len(t.Tags))


Suggested change

var result = make([]string, 0, 8*len(t.Tags))

var result = make([]string, 0, 2*len(t.Tags))

i think * 2 enough

zouyx · 2020-08-05T15:24:44Z

cluster/router/tag/router_rule.go

+	for _, t := range t.Tags {
+		if tag == t.Name {
+			return true
+		}
+	}
+	return false


Suggested change

for _, t := range t.Tags {

if tag == t.Name {

return true

}

}

return false

return len(tagNameToAddresses[tag])>0

Why dont you use tagNameToAddresses map[string][]string?

zouyx · 2020-08-05T15:26:48Z

cluster/router/tag/tag_router.go

+	routerRule := *c.tagRouterRule
+	return routerRule


Suggested change

routerRule := *c.tagRouterRule

return routerRule

return *c.tagRouterRule

enough? and i think no need to extract a new function for single scene

zouyx · 2020-08-05T15:30:23Z

cluster/router/tag/tag_router.go

+				if invoker.GetUrl().GetParam(constant.Tagkey, "") == "" {
+					return true
+				}
+				return false


Suggested change

if invoker.GetUrl().GetParam(constant.Tagkey, "") == "" {

return true

}

return false

return invoker.GetUrl().GetParam(constant.Tagkey, "") == ""

zouyx · 2020-08-05T15:32:10Z

cluster/router/tag/tag_router.go

+				if invoker.GetUrl().GetParam(constant.Tagkey, "") == tag {
+					return true
+				}
+				return false


Suggested change

if invoker.GetUrl().GetParam(constant.Tagkey, "") == tag {

return true

}

return false

return invoker.GetUrl().GetParam(constant.Tagkey, "") == tag

zouyx · 2020-08-05T15:32:32Z

cluster/router/tag/tag_router.go

+				if len(addresses) == 0 || !checkAddressMatch(tagRouterRuleCopy.getAddresses(), url.Ip, url.Port) {
+					return true
+				}
+				return false


Suggested change

if len(addresses) == 0 || !checkAddressMatch(tagRouterRuleCopy.getAddresses(), url.Ip, url.Port) {

return true

}

return false

return len(addresses) == 0 || !checkAddressMatch(tagRouterRuleCopy.getAddresses(), url.Ip, url.Port)

zouyx · 2020-08-05T15:32:57Z

cluster/router/tag/tag_router.go

+	if remoting.EventTypeDel == event.ConfigType {
+		c.tagRouterRule = nil
+		return
+	} else {


delete else

zouyx · 2020-08-05T15:33:55Z

cluster/router/tag/tag_router.go

+		if subnet != nil && subnet.Contains(net.ParseIP(host)) {
+			return true
+		}
+		return false


Suggested change

if subnet != nil && subnet.Contains(net.ParseIP(host)) {

return true

}

return false

return subnet != nil && subnet.Contains(net.ParseIP(host))

zouyx · 2020-08-05T15:34:55Z

cluster/router/tag/tag_router_test.go

 )

 import (
+	"github.com/dubbogo/go-zookeeper/zk"


split package

Ftr: add dynamic tag router

2e01ed5

watermelo changed the title ~~Ftr: dynamic tag router~~ [WIP]Ftr: dynamic tag router Jul 19, 2020

watermelo marked this pull request as draft July 19, 2020 14:48

watermelo changed the base branch from master to develop July 26, 2020 17:12

Add: add unit tests for tag router

71095a3

zouyx added this to the 1.5.1 milestone Jul 28, 2020

zouyx linked an issue Jul 28, 2020 that may be closed by this pull request

Align with Dubbo: Route based on tag #593

Closed

watermelo added 2 commits July 30, 2020 00:23

Add: add ip address match function

4ef21cb

Add: add listener for tag router

891ec6c

watermelo marked this pull request as ready for review July 29, 2020 17:30

Merge branch 'develop' into featrue/tagRouter

0d56ab0

watermelo changed the title ~~[WIP]Ftr: dynamic tag router~~ Ftr: dynamic tag router Jul 29, 2020

Mod: update tag router unit test

8580ad0

AlexStocks reviewed Jul 30, 2020

View reviewed changes

AlexStocks requested review from pantianying and zouyx July 30, 2020 05:14

zouyx reviewed Aug 2, 2020

View reviewed changes

watermelo added 2 commits August 3, 2020 01:46

Add: add unit test for dynamic tag

70c01a5

Add: add zk jar for tag test

a31a4e2

zouyx reviewed Aug 4, 2020

View reviewed changes

pantianying reviewed Aug 5, 2020

View reviewed changes

AlexStocks reviewed Aug 5, 2020

View reviewed changes

hxmhlt reviewed Aug 5, 2020

View reviewed changes

zouyx reviewed Aug 5, 2020

View reviewed changes

AlexStocks merged commit 5e377f4 into apache:develop Aug 6, 2020

AlexStocks mentioned this pull request Aug 6, 2020

Ftr: dynamic tag router #696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ftr: dynamic tag router #665

Ftr: dynamic tag router #665

watermelo commented Jul 19, 2020 •

edited

Loading

AlexStocks Jul 30, 2020

AlexStocks Jul 30, 2020

AlexStocks Jul 30, 2020

AlexStocks Jul 30, 2020

zouyx Aug 2, 2020

codecov-commenter commented Aug 3, 2020

zouyx left a comment

pantianying Aug 5, 2020

AlexStocks Aug 5, 2020

pantianying Aug 5, 2020

AlexStocks Aug 5, 2020

pantianying Aug 5, 2020

pantianying Aug 5, 2020

AlexStocks Aug 5, 2020

AlexStocks Aug 5, 2020

AlexStocks Aug 5, 2020

AlexStocks Aug 5, 2020

AlexStocks Aug 5, 2020

hxmhlt Aug 5, 2020

hxmhlt Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

zouyx Aug 5, 2020

		t.addressToTagNames = make(map[string][]string, 8)
		t.tagNameToAddresses = make(map[string][]string, 8)

-	t.addressToTagNames = make(map[string][]string, 8)
-	t.tagNameToAddresses = make(map[string][]string, 8)
+         l:=len(t.Tags)
+	t.addressToTagNames = make(map[string][]string, l*2)
+	t.tagNameToAddresses = make(map[string][]string, l)

	var result = make([]string, 0, 8*len(t.Tags))
	var result = make([]string, 0, 2*len(t.Tags))

	routerRule := *c.tagRouterRule
	return routerRule
	return *c.tagRouterRule

Ftr: dynamic tag router #665

Ftr: dynamic tag router #665

Conversation

watermelo commented Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 3, 2020

Codecov Report

zouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watermelo commented Jul 19, 2020 •

edited

Loading