Add option to let activities heartbeat during worker shutdown#2903
Add option to let activities heartbeat during worker shutdown#2903baekgyu-kim wants to merge 1 commit into
Conversation
maciejdudko
left a comment
There was a problem hiding this comment.
Hi @baekgyu-kim, thank you for your contribution! It's great to see someone taking on these long standing issues. However, this is not the right implementation.
There should be a new worker option to enable heartbeating during shutdown, It should default to disabled, and when disabled, the behavior should be identical to existing behavior for backward compatibility purposes.
When the option is enabled, the heartbeat behavior should be identical to what happens during normal heartbeat when the worker is not shutting down. There should be no additional code path that calls sendHeartbeatRequest a different way, the existing mechanism should be used. The way to achieve that is to modify SyncActivityWorker.shutdown so that heartbeatExecutor.shutdown is only called after all outstanding activity tasks have finished executing.
If you need assistance with implementation, feel free to reach out on community Slack, either message me directly or post on #java-sdk channel.
dc7f0cc to
0d8852a
Compare
|
Hi @maciejdudko, It now adds an experimental Whenever you have a chance, I'd appreciate another look. Thanks again! |
| * io.temporal.client.ActivityWorkerShutdownException}, unless {@link | ||
| * WorkerOptions.Builder#setActivityHeartbeatDuringShutdown(boolean)} is enabled, in which case | ||
| * heartbeats keep working until the activity tasks finish executing.<br> |
There was a problem hiding this comment.
shutdownNow behavior stays the same, see comment in SyncActivityWorker.
| * io.temporal.client.ActivityWorkerShutdownException}, unless {@link | |
| * WorkerOptions.Builder#setActivityHeartbeatDuringShutdown(boolean)} is enabled, in which case | |
| * heartbeats keep working until the activity tasks finish executing.<br> | |
| * io.temporal.client.ActivityWorkerShutdownException}.<br> |
There was a problem hiding this comment.
Reverted. The WorkerFactory.shutdown Javadoc no longer references the option, so shutdownNow keeps its original documented behavior.
31a0902 to
e5e60a0
Compare
|
Hi @maciejdudko, I've addressed all of your comments:
Thanks again for the careful review! |
maciejdudko
left a comment
There was a problem hiding this comment.
Almost there! Just one small change to a test remaining. Also, I see you removed the doc change for WorkerFactory.shutdown. I liked that one; I believe we should mention that shutdown behavior is different when allowActivityHeartbeatDuringShutdown is true (unlike shutdownNow which is the same both ways).
|
|
||
| // if heartbeating was incorrectly allowed, the activity would complete successfully and this | ||
| // assertion would fail | ||
| assertThrows(ActivityFailedException.class, handle::getResult); |
There was a problem hiding this comment.
Also assert that cause is ActivityWorkerShutdownException.
There was a problem hiding this comment.
- Restored the
WorkerFactory.shutdownjavadoc note that heartbeats keep working until the activity tasks finish whenallowActivityHeartbeatDuringShutdownis enabled. testHeartbeatingActivityFailsDuringShutdownNownow also asserts the cause by its type.
|
Hi @maciejdudko, Both points are addressed:
Thanks again for the careful reviews. Have a great day. |
83674c6 to
09fcf6b
Compare
What was changed
Added an experimental worker option,
WorkerOptions.Builder#setAllowActivityHeartbeatDuringShutdown(boolean)(defaultfalse, preserving the existing behavior).shutdownNowis used, the behavior is unchanged: the heartbeat executor is shut down first, soActivityExecutionContext.heartbeat()throwsActivityWorkerShutdownException.allowActivityHeartbeatDuringShutdown && !interruptTasks, so non-graceful shutdown (WorkerFactory.shutdownNow) always behaves as if the option were disabled.Touched:
WorkerOptions,SingleWorkerOptions,Worker,SyncActivityWorker.Why?
heartbeat()threwActivityWorkerShutdownExceptionfor the rest of theawaitTerminationgrace period. An activity that wanted to run to completion during that window could not refresh its server-side heartbeat deadline → the server timed it out and retried it → duplicate executions, even though the worker deliberately gave the activity time to finish.false, so existing behavior is preserved, andshutdownNowis intentionally excluded. Originates from Add the ability to keep heartbeating while the worker is shutting down #2075.Note: with the option enabled, activities are no longer notified of shutdown via
ActivityWorkerShutdownException, so they are expected to complete within the termination grace period on their own. This is documented on the setter.Checklist
Closes Add the ability to keep heartbeating while the worker is shutting down #2075
How was this tested:
New
HeartbeatDuringWorkerShutdownTest(standalone activity + two-semaphore handshake; gated behindSDKTestWorkflowRule.useExternalService):testHeartbeatingActivityCompletesDuringShutdown— option enabled + gracefulshutdown(): the activity heartbeats and runs to completion.testHeartbeatingActivityFailsDuringShutdownNow—shutdownNow(): the option is ignored, and the heartbeat fails the activity.WorkerOptionsTest— copy-builder round-trips the new option.Any docs updates needed?
WorkerOptions.Builder#setAllowActivityHeartbeatDuringShutdownJavadoc.