handle Retry-After header #151
Conversation
Codecov Report
@@ Coverage Diff @@
## master #151 +/- ##
============================================
+ Coverage 90.47% 90.55% +0.08%
- Complexity 128 137 +9
============================================
Files 42 42
Lines 399 413 +14
Branches 45 50 +5
============================================
+ Hits 361 374 +13
Misses 14 14
- Partials 24 25 +1
Continue to review full report at Codecov.
|
|
Hi @doyaaaaaken , I will be able to check this PR on weekend. |
There was a problem hiding this comment.
@doyaaaaaken
You're on a right way, please check my comments
| private val invocationTimeout: Long = 0, | ||
| private val ratio: Int = 1, | ||
| private val errorStatuses: List<Int> = listOf(503, 504) | ||
| private val failureThreshold: Int = 3, |
There was a problem hiding this comment.
Please avoid formatting change
| @@ -34,7 +35,13 @@ class RetryInterceptor( | |||
| delay = performAndReturnDelay(delay) | |||
| } | |||
| val response = chain.proceed(request) | |||
| if (!isRetry(response, attemptsCount)) return response | |||
|
|
|||
| val nextTime = calculateNextRetry(response, attemptsCount, delay, chain.readTimeoutMillis()) | |||
There was a problem hiding this comment.
There is a room for improvement here.
if () statement else return logic looks rather complicated
| @@ -34,7 +35,13 @@ class RetryInterceptor( | |||
| delay = performAndReturnDelay(delay) | |||
| } | |||
| val response = chain.proceed(request) | |||
| if (!isRetry(response, attemptsCount)) return response | |||
|
|
|||
| val nextTime = calculateNextRetry(response, attemptsCount, delay, chain.readTimeoutMillis()) | |||
There was a problem hiding this comment.
I suggest to have two stages here:
- check if we're going to retry ?
- calculate next delay.
Probably, we need to modify performAndReturnDelay
| internal fun isRetry(response: Response, attemptsCount: Int): Boolean = | ||
| attemptsCount < failureThreshold && response.code() in errorStatuses | ||
| internal fun calculateNextRetry(response: Response, attemptsCount: Int, delay: Long, maxDelayTime: Int): Long? { | ||
| val retryAfter = response.header("Retry-After")?.toLongOrNull() |
There was a problem hiding this comment.
probably need an ext function to get header by name.
BTW, did you check what is the metric of Retry-After values? s/ms ?
There was a problem hiding this comment.
probably need an ext function to get header by name.
OK!
BTW, did you check what is the metric of Retry-After values? s/ms ?
The unit of Retry-After value is seconds. But in my code it is millisecond.
Sorry, I'll fix it.
There was a problem hiding this comment.
no worries at all, that's why we're doing reviews :)
|
as for now, targeting to |
| internal fun isRetry(response: Response, attemptsCount: Int): Boolean = | ||
| attemptsCount < failureThreshold && response.code() in errorStatuses | ||
| internal fun calculateNextRetry(response: Response, attemptsCount: Int, delay: Long, maxDelayTime: Int): Long? { | ||
| val retryAfter = response.header("Retry-After")?.toLongOrNull()?.let { it * 1000 } |
|
I simplified logic according to this comment (#151 (comment)). And as I said in the first comment (#151 (comment)), below tasks are already left.
|
| internal fun parseRetryAfter(headers: Headers): Long? { | ||
| val retryAfter = headers["Retry-After"] ?: return null | ||
| return retryAfter.toLongOrNull()?.let { it * 1000 } ?: try { | ||
| val date = httpDateFormat.parse(retryAfter).toInstant().epochSecond |
There was a problem hiding this comment.
Working with time is tricky. I strongly suggest several things:
-
Do not do any custom calculations. with timestamps(epoch, etc )
-
Use joda time primitives. It will take care of all things like Arabic, Japanese, or Thai calendars.
val responseDate = ZonedDateTime.parse("...") val now = ZonedDateTime.now() val duration = Duration.between(responseDate, now) -
Please separate two stratagies in your code: 1st is to parse as number; 2nd is to parse as timestamp
| return retryAfter.toLongOrNull()?.let { it * 1000 } ?: try { | ||
| val date = httpDateFormat.parse(retryAfter).toInstant().epochSecond | ||
| val now = Instant.now().epochSecond | ||
| (date - now).takeIf { it > 0 } |
There was a problem hiding this comment.
It's great that you check it to be positive!
|
I fixes code about this comment! |
|
All fixes are done, so I remove 'WIP'. |
Fixes #105 #108
Read
Retry-Afterresponse header, and decide when to retry next.This is WIP pull request, so below tasks are not started.
Retry-Aftervalue. (GMT format https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After#Directives )Please review whether the direction is correct.