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

Fix/consul unreg #973

Merged
merged 6 commits into from
Jan 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions filter/filter_impl/sentinel_filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ func TestSentinelFilter_QPS(t *testing.T) {

_, err = flow.LoadRules([]*flow.Rule{
{
Resource: interfaceResourceName,
Resource: interfaceResourceName,
//MetricType: flow.QPS,
TokenCalculateStrategy: flow.Direct,
ControlBehavior: flow.Reject,
Threshold: 100,
Threshold: 100,
RelationStrategy: flow.CurrentResource,
},
})
Expand Down
39 changes: 23 additions & 16 deletions registry/consul/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package consul

import (
getty "github.com/apache/dubbo-getty"
"strconv"
"time"
)
Expand Down Expand Up @@ -56,6 +57,10 @@ type consulRegistry struct {
// Done field represents whether
// consul registry is closed.
done chan struct{}

// registeredURLs field represents all URLs that have been registered
// will be unregistered when destroyed
registeredURLs []*common.URL
}

func newConsulRegistry(url *common.URL) (registry.Registry, error) {
Expand Down Expand Up @@ -91,6 +96,7 @@ func (r *consulRegistry) Register(url *common.URL) error {

// register actually register the @url
func (r *consulRegistry) register(url *common.URL) error {
r.registeredURLs = append(r.registeredURLs, url.Clone())
Copy link

@JiaLiangoooo JiaLiangoooo Dec 28, 2020

Choose a reason for hiding this comment

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

问一个问题,在还没有注册consul之前就应该添加上去吗?

但是如果在注册成功之后,因为buildService(url)因为传进去的是url的引用,会修改掉url的参数,所以也会问题.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 我认为在注册前把url添加进数组没有问题。
    因为最终destroy的时候,即使unregister不存在的url也无妨。
  2. 因为buildServicce会对url进行修改,而其中buildid函数是根据修改前的url生成的id,在unregister的时候,也必须严格使用修改前的url来生成id,所以这里临时把修改前的拷贝了出来,用于unregister。

service, err := buildService(url)
if err != nil {
return err
Expand Down Expand Up @@ -188,25 +194,26 @@ func (r *consulRegistry) IsAvailable() bool {

// Destroy consul registry center
func (r *consulRegistry) Destroy() {
if r.URL != nil {
done := make(chan struct{}, 1)
go func() {
defer func() {
if e := recover(); e != nil {
logger.Errorf("consulRegistry destroy with panic: %v", e)
}
done <- struct{}{}
}()
if err := r.UnRegister(r.URL); err != nil {
logger.Errorf("consul registry unregister with err: %s", err.Error())
done := make(chan struct{}, 1)
go func() {
defer func() {
if e := recover(); e != nil {
logger.Errorf("consulRegistry destroy with panic: %v", e)
}
done <- struct{}{}
}()
select {
case <-done:
logger.Infof("consulRegistry unregister done")
case <-time.After(registryDestroyDefaultTimeout):
logger.Errorf("consul unregister timeout")
for _, url := range r.registeredURLs {
if err := r.UnRegister(url); err != nil {
logger.Errorf("consul registry unregister with err: %s", err.Error())
}
}
}()
select {
case <-done:
logger.Infof("consulRegistry unregister done")
case <-getty.GetTimeWheel().After(registryDestroyDefaultTimeout):
logger.Errorf("consul unregister timeout")
}

close(r.done)
}