Fixes an issue where last-heartbeat time was set to the first event's timestamp #3361
Conversation
| @@ -2074,7 +2074,6 @@ func (e *MutableStateImpl) ReplicateActivityTaskStartedEvent( | |||
| ai.StartedEventId = event.GetEventId() | |||
| ai.RequestId = attributes.GetRequestId() | |||
| ai.StartedTime = event.GetEventTime() | |||
| ai.LastHeartbeatUpdateTime = ai.StartedTime | |||
There was a problem hiding this comment.
There's another one in AddActivityTaskStartedEvent.
Also, we need to be very careful on this. and make sure it won't break sync activity task or heartbeat timeout calculation. Have you checked all usage of this field?
There was a problem hiding this comment.
I don't see any places relying on this being non-null in the value-read usages. It looks like they all use timestamp.TimeValue which has null checks. In DescribeWorkflowExecution, we are already only optionally setting that field. In timer_sequence:getActivityHeartbeatTimeout, we default to the start time if the LastHeartbeatUpdateTime is zero, which will have the same behavior. We also replace the field, but I don't see why that would cause any issues because the way it's read will still be the same usages as the ones I already checked.
There was a problem hiding this comment.
Also, thanks for catching the one in AddActivityTaskStartedEvent, fixed.
16ba94c to
6d7f8e9
Compare
…s LastHeartbeatTime to the event time
f825283 to
3508fcc
Compare
This PR fixes a bug where the first event for a pending activity would set its LastHeartbeatTime to the event time. I tested the fix by writing a test which calls the
ReplicateActivityTaskStartedEventmethod which had this issue, and verifies that theLastHeartbeatDetailsare nil (as long as they were previously nil). The biggest risk to this change is if someone was depending onLastHeartbeatDetailson their pending activities being non-nil.