Handle throttling in FileDescriptor implementations - #1149
Conversation
| // Matched ahead of the shape-specific cases below, since a slow down is reported both as a service error and as a | ||
| // client-side error, depending on the client in use. |
There was a problem hiding this comment.
If it's not too much of an hassle, can we leave it a small note regarding the behavior with the CRT client? That we observed with this being not a retryable exception.
| jitter: FiniteDuration = 1.second | ||
| ): FiniteDuration = { | ||
| val exponential = (base.toMillis * Math.pow(factor, attempt.toDouble)).toLong | ||
| val capped = max.fold(exponential)(m => Math.min(exponential, m.toMillis)) | ||
| (capped + (Random.nextDouble() * jitter.toMillis).toLong).millis |
There was a problem hiding this comment.
I wonder about this jitter approach. Reading through https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/, I think we could do better.
The default is 1.0 second currently and it's add after the capped value, so, to deal with contention from multiple callers, it will kind-of evenly spread things out at a maximum of like 1sec at the maximum, so for hundreds of callers, they will fall on the same second.
There's a few different ways we can take it. If we commit to a "full jitter" implementation we can simplify this a lot and remove the jitter argument and just do something like: ThreadLocalRandom.current().nextLong(capMillis + 1).millis
However, if we want to be able to toggle the jitterness, maybe something like:
val jitterFraction = Math.min(1.0, Math.max(0.0, jitter)) // being extra careful, lol...
(
(1.0 - jitterFraction) * capped +
ThreadLocalRandom.current().nextDouble() * jitterFraction * capped
).toLong.millisThis seems like a better alternative, but I didn't test or benchmarks it. Thoughts?
| jitter: FiniteDuration = 1.second | ||
| ): FiniteDuration = { | ||
| val exponential = (base.toMillis * Math.pow(factor, attempt.toDouble)).toLong | ||
| val capped = max.fold(exponential)(m => Math.min(exponential, m.toMillis)) | ||
| (capped + (Random.nextDouble() * jitter.toMillis).toLong).millis |
There was a problem hiding this comment.
Reading the javadocs of ThreadLocalRandom leads me to think that we want to prefer it here ThreadLocalRandom.current().
| private[aws] val BaseBackOff = 3.seconds | ||
| private[aws] val MaxBackOff = 30.seconds | ||
| private[aws] val BackOffJitter = 1.second | ||
|
|
There was a problem hiding this comment.
We could consider making these values also configurable, via HOCON, in the scenario we detect we might be "stuck" a long time retrying things.
I wouldn't block merging this PR because of that, as I think these new settings, with the default of two retries, don't change the status quo.
| } else { | ||
| val delay = exponentialBackOffDelay(attempt, base, max, factor, jitter) | ||
| onRetry(ex, delay, maxRetries - attempt) | ||
| Thread.sleep(delay.toMillis) |
There was a problem hiding this comment.
I came across this blog https://blog.damavis.com/en/blocking-calls-and-asynchronous-programming-with-scala/ and thought to share.
I don't think it matter that much for the current state of things, as apso is providing a "sync" API so this wouldn't have an effect on the apps using it.
| onRetry = (ex, delay, remaining) => | ||
| logger.warn(s"Error during S3 operation. Retrying in ${delay.toMillis}ms ($remaining more times)", ex), | ||
| onMaxRetriesReached = ex => logger.error("Max retries reached. Aborting S3 operation", ex) |
There was a problem hiding this comment.
Doesn't this mean that we are now logging the full exception three times? On handler, onRetry, and onMaxRetriesReached. This might be costly.
We could just log it on the handler.
| base = S3Bucket.BaseBackOff, | ||
| max = Some(S3Bucket.MaxBackOff), | ||
| jitter = S3Bucket.BackOffJitter, | ||
| retryWhen = !handler(_), |
There was a problem hiding this comment.
handler is a partial function (it already was before this PR), but it's being called normally. It might make sense to add a safeguard here, something like: ex => !handler.applyOrElse(ex, (_: Throwable) => false), to make a "bug" fail-fast.
2 participants
This PR:
Retry#exponentialBackOff.S3FileDescriptorandGCSFileDescriptorto use exponential backoff when retrying.ex.isRetryablequeries inGCSBucket.S3FileDescriptornow retries throttling exceptions by default (disable withaws.s3.retry-on-slow-down = false).Does this change relate to existing issues or pull requests?
NA
Does this change require an update to the documentation?
Yes - documented the typesafe config keys of the
S3FileDescriptor.How has this been tested?
Added
RetrySpecandS3BucketSpec.