-
Notifications
You must be signed in to change notification settings - Fork 220
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
Support configurable/default heartbeat default/max intervals. #660
Conversation
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.
Overall LGTM.
internal/internal_task_handlers.go
Outdated
} else if ath.defaultHeartbeatThrottleInterval > 0 { | ||
heartbeatThrottleInterval = ath.defaultHeartbeatThrottleInterval | ||
} else { | ||
heartbeatThrottleInterval = defaultDefaultHeartbeatThrottleInterval | ||
} | ||
// Limit interval to a max | ||
if ath.maxHeartbeatThrottleInterval > 0 && heartbeatThrottleInterval > ath.maxHeartbeatThrottleInterval { | ||
heartbeatThrottleInterval = ath.maxHeartbeatThrottleInterval | ||
} else if ath.maxHeartbeatThrottleInterval == 0 && heartbeatThrottleInterval > defaultMaxHeartbeatThrottleInterval { | ||
heartbeatThrottleInterval = defaultMaxHeartbeatThrottleInterval | ||
} |
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 find this a bit complicated to read.
It would have been easier to only have to consider ActivityOptions.heartbeatTimeout
, WorkerOptions.{default,max}HeartbeatThrottleInterval
and have defaults for those options provided outside of this method.
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.
They are provided outside this method too. But there are a couple of ways this is called in test code, so I wanted to do it here just to be safe.
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.
So I'd split it into 2 methods, one that sets the default options and one that does the calculation to reduce the complexity.
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.
Those methods would be a few lines and each called only once.
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.
Can we do something like this?
In func newActivityTaskHandlerWithCustomProvider
do:
defaultHeartbeatThrottleInterval:
providedOrDefaultInterval(params.DefaultHeartbeatThrottleInterval, defaultDefaultHeartbeatThrottleInterval),
maxHeartbeatThrottleInterval:
providedOrDefaultInterval(params.MaxHeartbeatThrottleInterval, defaultMaxHeartbeatThrottleInterval),
And make getHeartbeatThrottleInterval
something like:
if heartbeatTimeout > 0 {
heartbeatThrottleInterval = time.Duration(0.8 * float64(heartbeatTimeout))
} else {
heartbeatThrottleInterval = ath.defaultHeartbeatThrottleInterval
}
if heartbeatThrottleInterval > ath.maxHeartbeatThrottleInterval {
heartbeatThrottleInterval = ath.maxHeartbeatThrottleInterval
}
return heartbeatThrottleInterval
If we're missing the initialization in tests just make sure ActivityTaskHandler is properly initialized.
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 have changed this to separate the obtaining of interval (from timeout or fall back to configured or fall back to system) and max (from configured or fall back to system) and then to the check after that. Hopefully this is clearer.
…hrottle # Conflicts: # internal/activity_test.go # internal/internal_task_handlers.go
What was changed
Currently in Go the frequency pending heartbeats are relayed to the server are 80% of the given heartbeat timeout or if no heartbeat timeout, 8 minutes (80% of 10 minutes). This changes that to be 80% of given heartbeat timeout or 30 seconds and caps it at a configurable max, or if the max is not configured, 60 seconds.
This does mean users who previously had heartbeats way less frequent, e.g. every few minutes, will now have them more frequent (default no less frequent than 60s). We do not believe going down from the large default is particularly harmful to running users though it technically could cause more traffic.
Why?
It was decided all SDKs should behave the same with regards to heartbeat frequency. See #656 and its associated links.
Checklist