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): unable to recreate after canceled the consul watch #3162

Merged
merged 1 commit into from Mar 20, 2024

Conversation

harbourlga
Copy link
Contributor

Description (what this PR does / why we need it):

Which issue(s) this PR fixes (resolves / be part of):

Other special notes for the reviewers:

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jan 22, 2024
@harbourlga
Copy link
Contributor Author

When the idle mode ends, watch again does not continue to obtain the instance list of the service through consul.

@shenqidebaozi
Copy link
Sponsor Member

shenqidebaozi commented Jan 30, 2024

Can it be fixed when multiple clients are created for the same service? For example, if a client is idle, will b client also be unable to update the instance due to Watcher shutdown?

func (w *watcher) Stop() error {

@harbourlga
Copy link
Contributor Author

Can it be fixed when multiple clients are created for the same service? For example, if a client is idle, will b client also be unable to update the instance due to Watcher shutdown?

func (w *watcher) Stop() error {

If you are talking about multiple grpc clients, then each client will build a different resolver to watch name resolution updates. Each resolver has its own context to control the shutdown of the Watcher. Therefore, when client a becomes idle, client b will not close the watcher.

When creating the grpc client, a resolver will be built:
https://github.com/grpc/grpc-go/blob/02858ee5064040458f7e3e04e10765a62814c50b/resolver_wrapper.go#L80

ctx, cancel := context.WithCancel(context.Background())

w, err := b.discoverer.Watch(ctx, strings.TrimPrefix(target.URL.Path, "/"))

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Feb 1, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.72%. Comparing base (9106991) to head (658b5af).
Report is 15 commits behind head on main.

❗ Current head 658b5af differs from pull request most recent head 7437ac0. Consider uploading reports for the commit 7437ac0 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3162      +/-   ##
==========================================
+ Coverage   84.62%   84.72%   +0.10%     
==========================================
  Files          88       88              
  Lines        3993     3993              
==========================================
+ Hits         3379     3383       +4     
+ Misses        440      438       -2     
+ Partials      174      172       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…est instance of the service

* Add test cases

* Delete serviceSet when serviceSet has no watcher

* The context of resolve is controlled independently

* resolve use set context

* Remove test declare and do not use it

* gofmt

* golangci-lint fix
@dosubot dosubot bot added the LGTM label Mar 19, 2024
@shenqidebaozi shenqidebaozi changed the title fix: 当grpc结束闲置模式的时候,需要继续去获取服务的最新实例 fix(consul): unable to recreate after canceled the consul watch Mar 19, 2024
@daemon365 daemon365 merged commit 1fdaabb into go-kratos:main Mar 20, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
4 participants