Add lease duration to worker heartbeat request #661
Conversation
rkannan82
force-pushed
the
worker_heartbeat_lease_duration
branch
from
dffd5b8 to
a92e463
Compare
| // There are 3 states for a worker: Active, Inactive, and CleanedUp. | ||
| // Lifecycle transitions: | ||
| // - Active->Active: Each time the server receives a heartbeat from the worker, it will renew the lease and keep the worker in the active state. | ||
| // - Active->Inactive: When the lease expires, the server will consider the worker to be inactive, and reschedule activities that were known to be running as of that time. |
There was a problem hiding this comment.
Is this already true? If not we may want to go back and update the comment after it is, or, alternatively, let's hold off merging this until the implementation behind it is also ready.
Love the detail in the comment now though!
There was a problem hiding this comment.
No, none of this is implemented yet.
- Do we typically merge only after the server changes are merged/close to being merged? If so, what is the best practice for new feature development when we have cross repo dependencies like this?
- Can we just add a comment saying under development and merge?
- For context, later I will be evolving this proto to pass activities. Server side requires these protos for implementation.
There was a problem hiding this comment.
For all but the most obvious things, we usually would like an accompanying server implementation to ensure that the API matches what we want. We learn a lot during implementation and it can cause problems providing "under development" fields. We find that we so often cut releases of API that people will employ these models into their proxies and in other ways that makes backwards incompatible alterations problematic.
Would strongly suggest at least having server implementation built before merging this. For larger efforts, even with the server implementation built, we may want it in a separate branch so SDKs can design against it and we can apply any API learnings from that, but not sure this qualifies for such a need.
There was a problem hiding this comment.
Sounds good. I have converted this into a draft PR, so you can still review it. I will not be merging this.
Added a lease duration field to worker heartbeat request. If not set, or 0, the server will use a default value (1 minute).
This gives SDK the flexibility to determine the heartbeat timeout interval. The server will compute the lease expiry based on the time at which it receives the heartbeat.
No breaking changes. This field is currently not being used on the server.