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

Ftr: add dynamic tag router #703

Merged
merged 7 commits into from
Aug 15, 2020
Merged

Conversation

watermelo
Copy link
Member

What this PR does:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@watermelo watermelo changed the title Add: add setInvoker function for router chain Ftr: add dynamic tag router Aug 7, 2020
@zouyx zouyx added this to the 1.5.1 milestone Aug 8, 2020
cluster/router/router.go Outdated Show resolved Hide resolved
cluster/router/tag/tag_router.go Outdated Show resolved Hide resolved
@zouyx zouyx self-assigned this Aug 9, 2020
@zouyx zouyx added the enhancement New feature or request label Aug 9, 2020
@@ -36,5 +36,8 @@ cp ${zkJar} cluster/router/chain/zookeeper-4unittest/contrib/fatjar
mkdir -p cluster/router/condition/zookeeper-4unittest/contrib/fatjar
cp ${zkJar} cluster/router/condition/zookeeper-4unittest/contrib/fatjar

mkdir -p cluster/router/tag/zookeeper-4unittest/contrib/fatjar
cp ${zkJar} cluster/router/tag/zookeeper-4unittest/contrib/fatjar

Choose a reason for hiding this comment

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

if matchIp(address, host, port) {
return true
}
if address == net.JoinHostPort(constant.ANYHOST_VALUE, port) {

Choose a reason for hiding this comment

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

net.JoinHostPort(constant.ANYHOST_VALUE, port) is independent of the cycle, so it may be better to assign it to a value before cycle starts.

var invokers []protocol.Invoker
invokers = append(invokers, inv2, inv3, inv4, inv5)
filterTag := func(invoker protocol.Invoker) bool {
if invoker.GetUrl().GetParam(constant.Tagkey, "") == "beijing" {

Choose a reason for hiding this comment

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

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

assert.Equal(t, []protocol.Invoker{inv4, inv5}, res)
flag := true
filterEnabled := func(invoker protocol.Invoker) bool {
if invoker.GetUrl().GetParamBool(constant.RouterEnabled, false) == flag {

Choose a reason for hiding this comment

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

As above.

cluster/router/chain/chain.go Show resolved Hide resolved
// RouterRule RouterRule config read from config file or config center
type RouterRule struct {
router.BaseRouterRule `yaml:",inline"`
router.BaseRouterRule `yaml:",inline""`
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw three " , is this right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's old codes on purpose, when delete it will be parsed fail


// init use for flattening tags data to @addressToTagNames and @tagNameToAddresses
func (t *RouterRule) init() {
t.addressToTagNames = make(map[string][]string, 2*len(t.Tags))
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think named init is golang native. Because init is golang's special func. Maybe func (t *RouterRule)NewRouterRule is .

Name string
Addresses []string
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think golang need 'getXXX' .

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2020

Codecov Report

Merging #703 into develop will decrease coverage by 0.15%.
The diff coverage is 71.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #703      +/-   ##
===========================================
- Coverage    63.93%   63.77%   -0.16%     
===========================================
  Files          238      239       +1     
  Lines        12487    12778     +291     
===========================================
+ Hits          7983     8149     +166     
- Misses        3740     3833      +93     
- Partials       764      796      +32     
Impacted Files Coverage Δ
cluster/cluster_impl/base_cluster_invoker.go 72.15% <ø> (ø)
cluster/loadbalance/consistent_hash.go 90.32% <ø> (ø)
common/url.go 65.95% <0.00%> (ø)
common/yaml/yaml.go 86.66% <0.00%> (-13.34%) ⬇️
config/base_config.go 68.12% <ø> (ø)
config/provider_config.go 58.06% <0.00%> (ø)
protocol/dubbo/client.go 69.09% <ø> (ø)
protocol/dubbo/codec.go 72.50% <ø> (ø)
protocol/grpc/server.go 68.42% <ø> (ø)
protocol/result.go 0.00% <ø> (ø)
... and 36 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 25281bf...0a475bf. Read the comment docs.

// to false, which means it will invoke any providers without a tag unless it's explicitly disallowed.
if len(result) > 0 || isForceUseTag(url, invocation) {
return result
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

pls delete the else.

@AlexStocks AlexStocks merged commit 9d8afcd into apache:develop Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants