-
Notifications
You must be signed in to change notification settings - Fork 719
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
client: refine serviceModeKeeper code #6201
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,8 +91,11 @@ type tsoClient struct { | |
} | ||
|
||
// newTSOClient returns a new TSO client. | ||
func newTSOClient(ctx context.Context, cancel context.CancelFunc, option *option, keyspaceID uint32, | ||
svcDiscovery ServiceDiscovery, eventSrc tsoAllocatorEventSource, factory tsoStreamBuilderFactory) *tsoClient { | ||
func newTSOClient( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the code style I preferred when I use the other programming languages, such as c++ and Java, but the coding style I used in this code (on the left side) is referred to other places in the code base, though there seems to be multiple coding styles. Do we have a uniform coding style or the style just needs to pass go format check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no strong principles here, I just want to make the parameters group as lines to reduce the one-line length. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. I don't have strong preference. Your change here looks good to me. |
||
ctx context.Context, option *option, keyspaceID uint32, | ||
svcDiscovery ServiceDiscovery, factory tsoStreamBuilderFactory, | ||
) *tsoClient { | ||
ctx, cancel := context.WithCancel(ctx) | ||
c := &tsoClient{ | ||
ctx: ctx, | ||
cancel: cancel, | ||
|
@@ -105,6 +108,7 @@ func newTSOClient(ctx context.Context, cancel context.CancelFunc, option *option | |
updateTSOConnectionCtxsCh: make(chan struct{}, 1), | ||
} | ||
|
||
eventSrc := svcDiscovery.(tsoAllocatorEventSource) | ||
eventSrc.SetTSOLocalServAddrsUpdatedCallback(c.updateTSOLocalServAddrs) | ||
eventSrc.SetTSOGlobalServAddrUpdatedCallback(c.updateTSOGlobalServAddr) | ||
c.svcDiscovery.AddServiceAddrsSwitchedCallback(c.scheduleUpdateTSOConnectionCtxs) | ||
|
@@ -124,6 +128,9 @@ func (c *tsoClient) Setup() { | |
|
||
// Close closes the TSO client | ||
func (c *tsoClient) Close() { | ||
if c == nil { | ||
return | ||
} | ||
log.Info("closing tso client") | ||
|
||
c.cancel() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep this lock. Originally, I used the corresponding read lock in the getTSOClient, but eventually I used atomic.Value() to store tsoClient and removed the read lock. After that, I still decided to keep this lock and the critical section in the setServiceMode() for the following reasons:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the lock back. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. The change looks good to me.