Add android retry #131
Add android retry #131
Conversation
|
Hi @larkox - I've not been feeling well for the past few days. I hope you are not blocked on this review? |
There was a problem hiding this comment.
Also wondering if there is a way to test this by setting up a mock HTTP server or something? I was looking at https://github.com/firebase/firebase-admin-go/blob/v3.13.0/internal/http_client_test.go but they have access to the client's internal fields so it doesn't help us.
| defer cancelRetryContext() | ||
| _, err = me.client.Send(retryContext, fcmMsg) | ||
| if me.metrics != nil { | ||
| me.metrics.observerNotificationResponse(PushNotifyApple, time.Since(start).Seconds()) |
| if err == nil { | ||
| break | ||
| } | ||
|
|
||
| if !isRetryable(err) { | ||
| break | ||
| } |
There was a problem hiding this comment.
Can we combine these in an OR?
| if retries == MAX_RETRIES-1 { | ||
| me.logger.Errorf("Max retries reached did=%v", fcmMsg.Token) | ||
| break | ||
| } |
There was a problem hiding this comment.
Doesn't the for loop take care of this? Why do we need an extra check? And if we do need it, can we remove the condition from the for loop?
|
|
||
| select { | ||
| case <-generalContext.Done(): | ||
| case <-time.After(waitTime): |
There was a problem hiding this comment.
I think we need to incorporate some of this logic as well: https://github.com/firebase/firebase-admin-go/blob/54b81142d35e1b98cfd8687a9e56712c78a0f4ec/internal/http_client.go#L427-L438. We need to get the value of the Retry-After header and set that to be the minimum wait time to retry. Otherwise we will get a rate limited error again.
Summary
Add android retry to push proxy
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-56827