RUM-5977 create UploadSchedulerStrategy interface and default implementation #2222
Conversation
xgouchet
force-pushed
the
xgouchet/RUM-5977/custom_upload_scheduler_strategy
branch
from
1c08628 to
8663f6b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2222 +/- ##
===========================================
+ Coverage 69.97% 69.99% +0.02%
===========================================
Files 726 727 +1
Lines 27033 27042 +9
Branches 4555 4557 +2
===========================================
+ Hits 18915 18927 +12
+ Misses 6838 6836 -2
+ Partials 1280 1279 -1
|
| throwable: Throwable? | ||
| ): Long { | ||
| if (uploadAttempts > 0 && throwable == null && lastStatusCode == HTTP_ACCEPTED) { | ||
| decreaseInterval() |
There was a problem hiding this comment.
Question, if the upload has succeeded after some failed attempts, why not just recover to the default delay immediately?
There was a problem hiding this comment.
So far I've kept the historical behaviour of our backoff strategy. We will have future tickets to make a different implementations
xgouchet
force-pushed
the
xgouchet/RUM-5977/custom_upload_scheduler_strategy
branch
2 times, most recently
from
f36165d to
04b96ad
Compare
| interface UploadSchedulerStrategy { | ||
|
|
||
| /** | ||
| * Should return the delay in millisecond to wait until the next upload attempt |
There was a problem hiding this comment.
| * Should return the delay in millisecond to wait until the next upload attempt | |
| * Should return the delay in milliseconds to wait until the next upload attempt |
| /** | ||
| * Should return the delay in millisecond to wait until the next upload attempt | ||
| * is performed. | ||
| * @param featureName the name of the feature for which a new upload will be scheduled |
There was a problem hiding this comment.
since we are exposing feature name here, maybe we should mention that they are listed in the Feature.Companion object of the Feature interface?
| * Should return the delay in millisecond to wait until the next upload attempt | ||
| * is performed. | ||
| * @param featureName the name of the feature for which a new upload will be scheduled | ||
| * @param uploadAttempts the number of requests that were attempted during this run. Will be zero if the device |
There was a problem hiding this comment.
Maybe we should elaborate what is the meaning of the run here? Is it for a single batch or for all batches in general?
| * is not ready (e.g.: when offline or with low battery). If multiple batches can be uploaded, the attempts will | ||
| * stop at the first failure. | ||
| * @param lastStatusCode the HTTP status code of the last request (if available). A successful upload will have a | ||
| * status code 202 (Accepted). When null, it means that |
| * is not ready (e.g.: when offline or with low battery). If multiple batches can be uploaded, the attempts will | ||
| * stop at the first failure. | ||
| * @param lastStatusCode the HTTP status code of the last request (if available). A successful upload will have a | ||
| * status code 202 (Accepted). When null, it means that |
There was a problem hiding this comment.
side note: One of the dangers of exposing this interface (if it is exposed to the user configuration, apart of messing up with time unit) is that implementations may wrongly check for 200 status code instead.
There was a problem hiding this comment.
This will be exposed, but we won't document it in the website, so almost no one will implement it. Also it is mentioned in the documentation that they should check for a 202.
| lastBatchUploadStatus?.code, | ||
| lastBatchUploadStatus?.throwable |
There was a problem hiding this comment.
What if we simply pass lastBatchUploadStatus as a whole here? Does it have a downside?
There was a problem hiding this comment.
I'm not keen of making the whole UploadStatus public. WDYT?
| internal val uploadConfiguration: DataUploadConfiguration | ||
| ) : UploadSchedulerStrategy { | ||
|
|
||
| private val currentDelays = mutableMapOf<String, Long>() |
There was a problem hiding this comment.
We probably need to make this one thread-safe, just in case. Even though we have only a single upload thread currently, it may change in the future.
|
|
||
| // endregion | ||
|
|
||
| // region Internal |
xgouchet
force-pushed
the
xgouchet/RUM-5977/custom_upload_scheduler_strategy
branch
from
04b96ad to
e232d84
Compare
xgouchet
force-pushed
the
xgouchet/RUM-5977/custom_upload_scheduler_strategy
branch
from
e232d84 to
ad6e8f1
Compare
xgouchet
deleted the
xgouchet/RUM-5977/custom_upload_scheduler_strategy
branch
What does this PR do?
Creates a basic interface to let customers decide how long to wait after an upload before trying again, based on the number of upload attempts, status code and exceptions.