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: etcd registry lifecycle #3180

Merged

Conversation

Disdjj
Copy link
Contributor

@Disdjj Disdjj commented Jan 30, 2024

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

In the original code, the management of the "heartbeat" coroutine in the Registry relies on the "option.ctx" passed in from the outside. In the default case, this may lead to the "heartbeat" goroutine being unable to correctly determine whether it should end based on the state of the context during its execution.
In reality, in addition to using a timeout context for managing the Register and DeRegister methods, the Registry also needs a separate context to control its lifecycle.
This context should obviously be controlled by the Registry itself: initialized during Register and canceled during DeRegister.
This is the origin and purpose of this PR.

在原本的代码中, Registry的heartbeat协程的管理由外部传入的option.ctx管理, 在默认的情况下, 这有可能导致heartbeat goroutine在运行过程中无法正确的通过context 的状态来判断是否应该结束.
实际上, 除了在管理 Register 以及 DeRegister 两个方法超时的context 外, Registry还需要一个控制生命周期的ctx.
这个ctx显然应该由Registry自己来进行控制: 即在Register时进行初始化, 在Degister时进行cancel.
这就是这个PR的来源以及作用.

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

Other special notes for the reviewers:

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jan 30, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.54%. Comparing base (3110168) to head (f6a8f0e).

❗ Current head f6a8f0e differs from pull request most recent head 501efe8. Consider uploading reports for the commit 501efe8 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    #3180      +/-   ##
==========================================
- Coverage   81.64%   81.54%   -0.10%     
==========================================
  Files          91       91              
  Lines        4167     4167              
==========================================
- Hits         3402     3398       -4     
- Misses        587      589       +2     
- Partials      178      180       +2     

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

@dosubot dosubot bot added the LGTM label Mar 16, 2024
@shenqidebaozi shenqidebaozi changed the title [fix] etcd registry lifecycle fix: etcd registry lifecycle Mar 16, 2024
@demoManito demoManito force-pushed the fix/djj/etcd_registry_lifecycle branch from 96a7318 to 501efe8 Compare March 16, 2024 14:51
@shenqidebaozi shenqidebaozi merged commit 1ab258e into go-kratos:main Mar 16, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants