-
Notifications
You must be signed in to change notification settings - Fork 222
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
enhance background job management of region cache #1171
Conversation
Signed-off-by: zyguan <[email protected]>
Signed-off-by: zyguan <[email protected]>
Signed-off-by: zyguan <[email protected]>
@crazycs520 PTAL |
Signed-off-by: zyguan <[email protected]>
Signed-off-by: zyguan <[email protected]>
Signed-off-by: zyguan <[email protected]>
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.
LGTM
internal/locate/region_cache.go
Outdated
if r.closed() { | ||
return | ||
} | ||
ticker := time.NewTicker(interval) |
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.
How about moving this line after line 575? since the ticker is stop in another goroutine.
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.
My point is: if the interval <= 0, it panic when calling schedule, which is able to be recovered by the caller; however if we move it into the goroutine, panic cannot be caught by users.
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.
If so, we should check interval <= 0 here, instead of causing panic and recover to handle it?
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.
IMO, panic on interval <= 0 is reasonable, but the panic should be recoverable. I will update it as you said anyway.
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 think we should try to check in advance to avoid panic, Instead of letting it panic, after all, this is a system software, users will panic when they see panic logs.
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've changed it.
BTW, panic let you kown you've passed a wrong arg, however, keeping silent sometimes hide potential risks.
internal/locate/region_cache.go
Outdated
if r.closed() { | ||
return | ||
} | ||
ticker := time.NewTicker(interval) |
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.
ditto.
Signed-off-by: zyguan <[email protected]>
Signed-off-by: zyguan <[email protected]>
Signed-off-by: zyguan <[email protected]>
There are many background cron jobs in region cache. This PR tries to reduce boilerplate code (like the following) by
bgRunner
.ref: #1104