Introduce local timeout for activity heartbeats#2804
Introduce local timeout for activity heartbeats#2804Sushisource wants to merge 4 commits intotemporalio:masterfrom
Conversation
| private static final long DEFAULT_LOCAL_HEARTBEAT_TIMEOUT_BUFFER_MILLIS = 5000; | ||
|
|
||
| static long getLocalHeartbeatTimeoutBufferMillis() { | ||
| String envVal = System.getenv("TEMPORAL_ACTIVITY_TIMEOUT_DELAY"); |
There was a problem hiding this comment.
I did this b/c I didn't want a worker option for a setting I expect basically no one will ever care about adjusting. If we feel otherwise, can promote.
There was a problem hiding this comment.
Could use a Java property here, what we did for some other debug configs, but this does work as well
There was a problem hiding this comment.
Oh that's a good call let's do that.
There was a problem hiding this comment.
Oh does no test use this setting? I thought you needed this to make tests faster. If no test needs it I would just make it a constant.
| this.localHeartbeatTimeoutBufferMillis = localHeartbeatTimeoutBufferMillis; | ||
| if (this.heartbeatTimeoutMillis > 0) { | ||
| this.heartbeatTimeoutFuture = | ||
| heartbeatExecutor.schedule( |
There was a problem hiding this comment.
Is there a reason you did and executor and a future instead of just keeping track of the last time a heartbeat succeeded and throwing an exception if it exceeded the timeout?
There was a problem hiding this comment.
Throw from where? The heartbeating method itself? Good point. I did this because I was thinking we'd proactively cancel the activity a-la what Core does, but, that turns out not to work here without more refactoring.
Can certainly change it to not bother w/ a future, or could do the more elaborate thing. Former is probably sufficient and is easier.
2 participants
What was changed
See title
Why?
User request, matches Core behavior (though only for heartbeat timeouts, not overall activity timeouts)
Checklist
Closes Keep track of current heartbeat timeout while retrying failures in doHeartBeatLocked #2775
How was this tested:
Added test
Any docs updates needed?