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/10/12 07:26:49 UTC

[GitHub] [dubbo-go] LaurenceLiZhixin opened a new pull request #788: Fix/consul destory

LaurenceLiZhixin opened a new pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788


   What this PR does:
   修复consul Registry 在interrupt后不会立刻从注册中心消失的bug
   
   Which issue(s) this PR fixes:
   Fixes #749
   
   Special notes for your reviewer:
   修复consul Registry 在interrupt后不会立刻从注册中心消失的bug，在consulRegistry的Destory函数中增加对Unregister函数的调用。从而从consul反注册。增加超时unregister超时的处理。
   
   Does this PR introduce a user-facing change?:
   NONE


----------------------------------------------------------------
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 #788: Fix: consul destory

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788#discussion_r503339794



##########
File path: registry/consul/registry.go
##########
@@ -187,5 +192,27 @@ func (r *consulRegistry) IsAvailable() bool {
 
 // Destroy consul registry center
 func (r *consulRegistry) Destroy() {
+	if r.URL != nil {
+		done := make(chan struct{}, 1)
+		ticker := time.NewTicker(r.timeOut)
+		go func() {
+			defer func() {
+				if e := recover(); e != nil {
+					logger.Errorf("consulRegistry destory with panic: %v", e)
+				}
+				done <- struct{}{}
+			}()
+			if err := r.UnRegister(*r.URL); err != nil {
+				logger.Errorf("consul registry unregister with err: %s", err.Error())
+			}
+		}()
+		select {
+		case <-done:
+			logger.Infof("consulRegistry unregister done")
+		case <-ticker.C:

Review comment:
       a time.After is enough in this simple case.




----------------------------------------------------------------
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-io commented on pull request #788: Fix: consul destory

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788#issuecomment-707672400


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=h1) Report
   > Merging [#788](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/dubbo-go/commit/811f4f1a2dcc75b4294f62cb795da0f3f399be93?el=desc) will **decrease** coverage by `3.87%`.
   > The diff coverage is `52.74%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/788/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL)](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #788      +/-   ##
   ===========================================
   - Coverage    63.75%   59.87%   -3.88%     
   ===========================================
     Files          239      259      +20     
     Lines        12796    12753      -43     
   ===========================================
   - Hits          8158     7636     -522     
   - Misses        3849     4163     +314     
   - Partials       789      954     +165     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cluster/router/condition/listenable\_router.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvY29uZGl0aW9uL2xpc3RlbmFibGVfcm91dGVyLmdv) | `54.71% <0.00%> (-0.13%)` | :arrow_down: |
   | [cluster/router/tag/file.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL2ZpbGUuZ28=) | `76.19% <0.00%> (-0.74%)` | :arrow_down: |
   | [common/extension/health\_checker.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9oZWFsdGhfY2hlY2tlci5nbw==) | `50.00% <ø> (-16.67%)` | :arrow_down: |
   | [common/proxy/proxy\_factory/default.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29tbW9uL3Byb3h5L3Byb3h5X2ZhY3RvcnkvZGVmYXVsdC5nbw==) | `13.33% <0.00%> (-7.31%)` | :arrow_down: |
   | [common/rpc\_service.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29tbW9uL3JwY19zZXJ2aWNlLmdv) | `87.94% <ø> (+1.35%)` | :arrow_up: |
   | [config/consumer\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL2NvbnN1bWVyX2NvbmZpZy5nbw==) | `51.21% <ø> (-5.04%)` | :arrow_down: |
   | [config/provider\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL3Byb3ZpZGVyX2NvbmZpZy5nbw==) | `50.00% <ø> (-8.07%)` | :arrow_down: |
   | [config/reference\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL3JlZmVyZW5jZV9jb25maWcuZ28=) | `80.00% <ø> (+0.17%)` | :arrow_up: |
   | [config/remote\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL3JlbW90ZV9jb25maWcuZ28=) | `75.00% <ø> (+1.31%)` | :arrow_up: |
   | [config\_center/apollo/impl.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnX2NlbnRlci9hcG9sbG8vaW1wbC5nbw==) | `83.63% <ø> (+2.81%)` | :arrow_up: |
   | ... and [324 more](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/788?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/788?src=pr&el=footer). Last update [811f4f1...34968e6](https://codecov.io/gh/apache/dubbo-go/pull/788?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] codecov-io edited a comment on pull request #788: Fix: consul destory

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788#issuecomment-707672400


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=h1) Report
   > Merging [#788](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/dubbo-go/commit/811f4f1a2dcc75b4294f62cb795da0f3f399be93?el=desc) will **decrease** coverage by `3.87%`.
   > The diff coverage is `52.74%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/788/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL)](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #788      +/-   ##
   ===========================================
   - Coverage    63.75%   59.87%   -3.88%     
   ===========================================
     Files          239      259      +20     
     Lines        12796    12753      -43     
   ===========================================
   - Hits          8158     7636     -522     
   - Misses        3849     4163     +314     
   - Partials       789      954     +165     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/788?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cluster/router/condition/listenable\_router.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvY29uZGl0aW9uL2xpc3RlbmFibGVfcm91dGVyLmdv) | `54.71% <0.00%> (-0.13%)` | :arrow_down: |
   | [cluster/router/tag/file.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL2ZpbGUuZ28=) | `76.19% <0.00%> (-0.74%)` | :arrow_down: |
   | [common/extension/health\_checker.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9oZWFsdGhfY2hlY2tlci5nbw==) | `50.00% <ø> (-16.67%)` | :arrow_down: |
   | [common/proxy/proxy\_factory/default.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29tbW9uL3Byb3h5L3Byb3h5X2ZhY3RvcnkvZGVmYXVsdC5nbw==) | `13.33% <0.00%> (-7.31%)` | :arrow_down: |
   | [common/rpc\_service.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29tbW9uL3JwY19zZXJ2aWNlLmdv) | `87.94% <ø> (+1.35%)` | :arrow_up: |
   | [config/consumer\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL2NvbnN1bWVyX2NvbmZpZy5nbw==) | `51.21% <ø> (-5.04%)` | :arrow_down: |
   | [config/provider\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL3Byb3ZpZGVyX2NvbmZpZy5nbw==) | `50.00% <ø> (-8.07%)` | :arrow_down: |
   | [config/reference\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL3JlZmVyZW5jZV9jb25maWcuZ28=) | `80.00% <ø> (+0.17%)` | :arrow_up: |
   | [config/remote\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnL3JlbW90ZV9jb25maWcuZ28=) | `75.00% <ø> (+1.31%)` | :arrow_up: |
   | [config\_center/apollo/impl.go](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree#diff-Y29uZmlnX2NlbnRlci9hcG9sbG8vaW1wbC5nbw==) | `83.63% <ø> (+2.81%)` | :arrow_up: |
   | ... and [324 more](https://codecov.io/gh/apache/dubbo-go/pull/788/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/788?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/788?src=pr&el=footer). Last update [811f4f1...cdf7405](https://codecov.io/gh/apache/dubbo-go/pull/788?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] LaurenceLiZhixin commented on a change in pull request #788: Fix: consul destory

Posted by GitBox <gi...@apache.org>.
LaurenceLiZhixin commented on a change in pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788#discussion_r503354858



##########
File path: registry/consul/registry.go
##########
@@ -187,5 +192,27 @@ func (r *consulRegistry) IsAvailable() bool {
 
 // Destroy consul registry center
 func (r *consulRegistry) Destroy() {
+	if r.URL != nil {
+		done := make(chan struct{}, 1)
+		ticker := time.NewTicker(r.timeOut)
+		go func() {
+			defer func() {
+				if e := recover(); e != nil {
+					logger.Errorf("consulRegistry destory with panic: %v", e)
+				}
+				done <- struct{}{}
+			}()
+			if err := r.UnRegister(*r.URL); err != nil {
+				logger.Errorf("consul registry unregister with err: %s", err.Error())
+			}
+		}()
+		select {
+		case <-done:
+			logger.Infof("consulRegistry unregister done")
+		case <-ticker.C:

Review comment:
       Yes, I fixed it.




----------------------------------------------------------------
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] hxmhlt merged pull request #788: Fix: consul destory

Posted by GitBox <gi...@apache.org>.
hxmhlt merged pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788


   


----------------------------------------------------------------
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] cityiron commented on pull request #788: Fix: consul destory

Posted by GitBox <gi...@apache.org>.
cityiron commented on pull request #788:
URL: https://github.com/apache/dubbo-go/pull/788#issuecomment-707696902


   LGTM


----------------------------------------------------------------
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