Fix/consul unreg #973
Merged
Fix/consul unreg #973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #973 +/- ##
===========================================
- Coverage 59.92% 59.59% -0.34%
===========================================
Files 261 261
Lines 12915 12903 -12
===========================================
- Hits 7739 7689 -50
- Misses 4223 4247 +24
- Partials 953 967 +14
Continue to review full report at Codecov.
|
| @@ -91,6 +95,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()) | |||
There was a problem hiding this comment.
问一个问题，在还没有注册consul之前就应该添加上去吗？
但是如果在注册成功之后，因为buildService(url)因为传进去的是url的引用，会修改掉url的参数，所以也会问题.
There was a problem hiding this comment.
- 我认为在注册前把url添加进数组没有问题。
因为最终destroy的时候，即使unregister不存在的url也无妨。 - 因为buildServicce会对url进行修改，而其中buildid函数是根据修改前的url生成的id，在unregister的时候，也必须严格使用修改前的url来生成id，所以这里临时把修改前的拷贝了出来，用于unregister。
added 2 commits
December 29, 2020 15:05
AlexStocks
force-pushed
the
develop
branch
23 times, most recently
from
b6022bb to
9e40791
Compare
AlexStocks
reviewed
Jan 4, 2021
registry/consul/registry.go
Outdated
| select { | ||
| case <-done: | ||
| logger.Infof("consulRegistry unregister done") | ||
| case <-time.After(registryDestroyDefaultTimeout): |
There was a problem hiding this comment.
using getty.GetTimeWheel().After(registryDestroyDefaultTimeout) instead.
AlexStocks
force-pushed
the
develop
branch
4 times, most recently
from
ca6b03e to
34fa1da
Compare
gaoxinge
approved these changes
Jan 7, 2021
AlexStocks
force-pushed
the
develop
branch
3 times, most recently
from
d12bf6a to
e633a73
Compare
AlexStocks
approved these changes
Jan 9, 2021
fangyincheng
approved these changes
Jan 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
fix consul unregister bug.
Which issue(s) this PR fixes:
Fixes #749
Special notes for your reviewer:
I record the copy of every registry url, and make sure the unregistry url is we wanted.
Does this PR introduce a user-facing change?: