Update network retry behavior to extend retries, add jitter #148
Conversation
- Retry 50 times before failure - Use same timing logic for the "exponential" backoff with jitter - Re-worked the unit test based on better understanding of the thread Handler behavior
evan-masseau
requested review from
jordan-griffin,
kennyklaviyo,
ndurell and
ajaysubra
evan-masseau
added
enhancement
sdk/analytics/src/main/java/com/klaviyo/analytics/networking/KlaviyoApiClient.kt
Show resolved
Hide resolved
| handler?.postDelayed(this, flushInterval) | ||
| } | ||
|
|
||
| private fun computeRetryInterval(attempts: Int): Long { |
There was a problem hiding this comment.
Logic here looks totally sound. Just wondering if this is a mirror of what we do on iOS? Like are the limits we apply to our jitter the same, etc.
There was a problem hiding this comment.
Default min/max retry intervals as well
There was a problem hiding this comment.
Yes I believe so. Would like a confirmation of my reading from Noah or Ajay. The difference is mostly just that I compute the retry time all in one function, whereas on iOS its split up into a couple different parts of the network queueing code.
There was a problem hiding this comment.
@ndurell @ajaysubra when one of you has a chance ^
There was a problem hiding this comment.
I took a gander at the iOS code and this looks inline. Looks like based on this message that Evan posted, we have at least 7 attempts before hitting the 3 min threshold if the app is active. This lines up with that.
There was a problem hiding this comment.
Thanks for the verification!
sdk/core/src/main/java/com/klaviyo/core/config/KlaviyoConfig.kt
Outdated
Show resolved
Hide resolved
| private fun computeRetryInterval(attempts: Int): Long { | ||
| val minRetryInterval = flushInterval | ||
| val jitterSeconds = Registry.config.networkJitterRange.random() | ||
| val exponentialBackoff = (2.0.pow(attempts).toLong() + jitterSeconds).times(1_000) |
There was a problem hiding this comment.
I'm guessing the 1000 converts seconds to milliseconds?
| handler?.postDelayed(this, flushInterval) | ||
| } | ||
|
|
||
| private fun computeRetryInterval(attempts: Int): Long { |
There was a problem hiding this comment.
I took a gander at the iOS code and this looks inline. Looks like based on this message that Evan posted, we have at least 7 attempts before hitting the 3 min threshold if the app is active. This lines up with that.
Description
This should match the Swift SDK retry behavior as I understood it. I tried to make the test very readable, would appreciate @ndurell or @ajaysubra confirming my logic.
First of two PRs regarding network rate limits.
Check List
Changelog / Code Overview
Test Plan
Related Issues/Tickets
CHNL-6779