[Improve] Handle HTTP header Retry-After from responses from OneSignal #2064
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkasten2
force-pushed
the
improve/handle-http-response-header-retry-after
branch
2 times, most recently
from
f38d522 to
533d321
Compare
jkasten2
requested review from
nan-li,
shepherd-l,
jennantilla,
brismithers and
jinliu9508
jkasten2
changed the title
jkasten2
changed the title
nan-li
requested changes
Apr 30, 2024
| @@ -37,6 +37,11 @@ class ExecutionResponse( | |||
| * When specified, any operations that should be prepended to the operation repo. | |||
| */ | |||
| val operations: List<Operation>? = null, | |||
| /** | |||
| * Optional Integer value maybe returned from the backend. | |||
| * Any future requests by this time. | |||
There was a problem hiding this comment.
nit - is this missing first part of "The module handing this should delay any future requests by this time."
There was a problem hiding this comment.
ah good catch, just fixed this with a git commit --fixup and then did a git rebase --autosquash to keep the commits clean. New commit is 0e1f4a9
| @@ -12,4 +12,9 @@ class BackendException( | |||
| * The response, if one exists. | |||
| */ | |||
| val response: String? = null, | |||
| /** | |||
| * Optional Integer value maybe returned from the backend. | |||
| * Any future requests by this time. | |||
There was a problem hiding this comment.
nit - is this missing first part of "The module handing this should delay any future requests by this time."
There was a problem hiding this comment.
ah good catch, just fixed this with a git commit --fixup and then did a git rebase --autosquash to keep the commits clean. New commit is 0e1f4a9
nan-li
approved these changes
Apr 30, 2024
|
I confused myself with seconds and milliseconds. Approved now! |
Add all the framework to parse Retry-After from the HTTP client layer, through to the code calling it, to the OperationRepo. This commit doesn't change any of the logic yet, but gets everything in place before making behavior changes and adding the tests.
The real getHeaderField supports returning null when the key is not set. Correcting this fixes failing HTTPClientTest
Use the retryAfterSeconds to stall processing in the OperationRepo. We wait in the OperationRepo as this allows more operations to be grouped together, as we getGroupableOperations will be called after the delay. There are number of other places that use HTTPClient other than the OperationRepo, we will address those in a follow up commit.
When we get a Retry-After we delay all future requests until after this time as passed. To be safe we assume the server's Retry-After value means all traffic should be delayed until then. If we wanted finer grain control we could work with the the backend to decide which endpoints should be affected.
jkasten2
force-pushed
the
improve/handle-http-response-header-retry-after
branch
from
533d321 to
0a8684b
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Respect standard HTTP Retry-After header from any responses from OneSignal's backend.
Details
Motivation
When OneSignal's backend is under heavy load they want to be able to throttle back requests to them.
Scope
Only effects delaying follow up network requests after we receive
Retry-Afterin a response. IfOpeartionRepotriggers this then it handles this smartly by delaying future requests, so more batching can be done. Other code that depends on theHttpClientsimply delays requests to respect this header.Testing
Unit testing
Added and updated a number of Unit tests. OperationRepo, it's executors, and HttpClientTests.
Manual testing
Tested on Android 14 emulator. Ensured we correctly handled 429 with it's
Retry-Afterby doing the following.429Affected code checklist
Checklist
Overview
Testing
Final pass
This change is