-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[New Scheduler] Implement KeepAliveService #5067
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5067 +/- ##
===========================================
- Coverage 81.61% 35.54% -46.08%
===========================================
Files 204 205 +1
Lines 9950 10011 +61
Branches 447 457 +10
===========================================
- Hits 8121 3558 -4563
- Misses 1829 6453 +4624
Continue to review full report at Codecov.
|
case class Lease(id: Long, ttl: Long) extends KeepAliveServiceData | ||
|
||
// Events received by the actor | ||
case object ReGrantLease |
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.
case object ReGrantLease | |
case object RegrantLease |
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 updated it to RegrantLease
implicit val ec: ExecutionContextExecutor = context.dispatcher | ||
|
||
private val leaseTimeout = loadConfigOrThrow[Int](ConfigKeys.etcdLeaseTimeout).seconds | ||
private var worker: Option[Cancellable] = None |
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'm not sure if this var is thread safe since it gets set in a Future
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.
By including the worker in the state data, I removed the var value.
LGTM |
I've reflected all of your reviews. please review again :) |
I would merge this in 24 hours for subsequent PRs. |
LGTM |
Description
Related issue and scope
My changes affect the following components
Types of changes
Checklist: