client/sd: unify the service discovery callbacks within a struct #9014
Conversation
ti-chi-bot
bot
added
release-note-none
JmPotato
force-pushed
the
sd_callback
branch
3 times, most recently
from
be379e4 to
594be8e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9014 +/- ##
==========================================
+ Coverage 76.22% 76.32% +0.09%
==========================================
Files 465 466 +1
Lines 70682 70727 +45
==========================================
+ Hits 53879 53984 +105
+ Misses 13445 13388 -57
+ Partials 3358 3355 -3
Flags with carried forward coverage won't be shown. Click here to find out more. |
ti-chi-bot
bot
added
needs-1-more-lgtm
|
/test pull-integration-realcluster-test |
pd/tests/integrations/realcluster/cluster.go Lines 141 to 152 in 093b841 In the test report, it download the binaries when deploy ms cluster, which is unexpected because binaries exist🤔, maybe it is a tiup bug. |
|
|
||
| // serviceCallbacks contains all the callback functions for service discovery events | ||
| type serviceCallbacks struct { | ||
| sync.RWMutex |
There was a problem hiding this comment.
This lock will be used in the next PR.
| @@ -126,14 +126,16 @@ type ServiceDiscovery interface { | |||
| // CheckMemberChanged immediately check if there is any membership change among the leader/followers | |||
| // in a quorum-based cluster or among the primary/secondaries in a primary/secondary configured cluster. | |||
| CheckMemberChanged() error | |||
| // ExecuteAndAddServingURLSwitchedCallback executes the callback once and adds it to the callback list then. | |||
| ExecuteAndAddServingURLSwitchedCallback(cb leaderSwitchedCallbackFunc) | |||
There was a problem hiding this comment.
Why we need a ExecuteAndAddServingURLSwitchedCallback interface?
If we need execute some functions, I think we can execute them directly.
There was a problem hiding this comment.
serviceDiscovery is exposed as an interface, the caller cannot access those internal callbacks.
There was a problem hiding this comment.
Because I find it is called in only one place. So maybe Execute is an unnecessary abstraction.
There was a problem hiding this comment.
It will be used in the next PR also, this method is useful if we want to integrate a sub-client into the client, an immediate execution of the callback with the lastest leader URL is necessary, e.g., https://github.com/tikv/pd/pull/8939/files#diff-c2cd9790c2f82b378808c8f7326f85b46ab36aa388c95250865571df80a4679bR193.
| @@ -0,0 +1,104 @@ | |||
| // Copyright 2025 TiKV Project Authors. | |||
There was a problem hiding this comment.
How about moving this file to pkg?
There was a problem hiding this comment.
Its code is highly related with the service discovery, pkg is for those components which are more common.
Signed-off-by: JmPotato <[email protected]>
Signed-off-by: JmPotato <[email protected]>
Signed-off-by: JmPotato <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: okJiang, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: ref #8690.
What is changed and how does it work?
Check List
Tests
Release note