Implementing retry once for FIS Client. #895
Merged
Conversation
This file contains hidden or 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
ciarand
reviewed
Oct 10, 2019
| @@ -53,8 +55,6 @@ | |||
| private static final String CONTENT_ENCODING_HEADER_KEY = "Content-Encoding"; | |||
| private static final String GZIP_CONTENT_ENCODING = "gzip"; | |||
|
|
|||
| private static final String UNAUTHORIZED_ERROR_MESSAGE = | |||
| "The request did not have the required credentials."; | |||
| private static final String INTERNAL_SERVER_ERROR_MESSAGE = "There was an internal server error."; | |||
| private static final String NETWORK_ERROR_MESSAGE = "The server returned an unexpected error:"; | |||
There was a problem hiding this comment.
missing a space between the colon and the error when you concat using +. It would be slightly less error prone to use either a format string here or have a dedicated method for generating these errors (or both)
...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
Outdated
Show resolved
Hide resolved
| httpsURLConnection.addRequestProperty(CONTENT_TYPE_HEADER_KEY, JSON_CONTENT_TYPE); | ||
| httpsURLConnection.addRequestProperty(ACCEPT_HEADER_KEY, JSON_CONTENT_TYPE); | ||
| httpsURLConnection.addRequestProperty(CONTENT_ENCODING_HEADER_KEY, GZIP_CONTENT_ENCODING); | ||
| HttpsURLConnection httpsURLConnection = (HttpsURLConnection) url.openConnection(); |
There was a problem hiding this comment.
there's a lot of overlap between this method and the other one that sets up a POST request. Can we maybe consolidate some of the shared logic?
...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
Show resolved
Hide resolved
* Simplifying FirebaseInstallations class by adding listeners. * Addressing ciaran's comments to return same token if multiple getAuthToken() calls are triggered simultaneously. * Cleaning doRegistration method. * Fixing FISClient to correctly parse expiration timestamp. (#848) * Updating getAuthToken to return creation timestamp (#884) * Propagating the exceptions to the clients. (#856)
|
/retest |
ciarand
approved these changes
Oct 10, 2019
| throw new FirebaseInstallationServiceException( | ||
| NETWORK_ERROR_MESSAGE + e.getMessage(), | ||
| FirebaseInstallationServiceException.Status.NETWORK_ERROR); | ||
| throw new FirebaseException(String.format(NETWORK_ERROR_MESSAGE, e.getMessage())); |
There was a problem hiding this comment.
does it make sense to embed the IOException as a cause?
There was a problem hiding this comment.
IOException wraps many exceptions : MalformedException, ProtocolException, UnknownServiceException etc. My reasoning to add this as a cause for users to know what went wrong and handle accordingly.
...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java
Show resolved
Hide resolved
|
/retest |
|
/test new-smoke-tests |
1 similar comment
|
/test new-smoke-tests |
|
/retest |
|
/test new-smoke-tests |
ankitaj224
added a commit
that referenced
this pull request
Oct 14, 2019
* Fixing FISClient to correctly parse expiration timestamp. (#848) * Updating getAuthToken to return creation timestamp (#884) * Propagating the exceptions to the clients. (#856) * Extract FID from FIS createInstallation response (#888) * Addressing Rayo's comment: Rename PersistedFidEntry to (#899) * Implementing retry once for FIS Client. (#895) * Simplifying FirebaseInstallations class by adding listeners. (#847) * Fixing FISClient to correctly parse expiration timestamp. (#848) * Updating getAuthToken to return creation timestamp (#884) * Propagating the exceptions to the clients. (#856)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.