Skip to content

Patch caching fid in memory on successful registration. #1764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jul 9, 2020
Merged

Patch caching fid in memory on successful registration. #1764

merged 6 commits into from
Jul 9, 2020

Conversation

ankitaj224
Copy link
Contributor

Also, updating caching FID logic due to the reverted PR.

@googlebot googlebot added the cla: yes Override cla label Jul 7, 2020
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 7, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • firebase-installations

    Type Base (0ce007a) Head (fa0ffc01) Diff
    aar 59.1 kB 59.1 kB -63 B (-0.1%)
    apk (debug) 801 kB 801 kB +201 B (+0.0%)
    apk (release) 648 kB 648 kB -112 B (-0.0%)

Test Logs

Notes

Head commit (fa0ffc01) is created by Prow via merging commits: 0ce007a 85910e3.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 7, 2020 •
edited
Loading

Coverage Report

Affected SDKs

  • firebase-installations

    SDK overall coverage changed from 57.30% (0ce007a) to 57.40% (fa0ffc01) by +0.10%.

    Filename Base (0ce007a) Head (fa0ffc01) Diff
    FirebaseInstallations.java 97.85% 97.87% +0.02%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (fa0ffc01) is created by Prow via merging commits: 0ce007a 85910e3.

andirayo
andirayo approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@andirayo andirayo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the multi-threading with the tasks, executors, and listeners my understanding is limited, unfortunately. :/

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated
@@ -322,7 +341,8 @@ private String doGetId() {
* @param forceRefresh true if this is for a getAuthToken call and if the caller wants to fetch a
* new auth token from the server even if an unexpired auth token exists on the client.
*/
private void doGetAuthToken(boolean forceRefresh) {
private final void doRegistrationInternal(boolean forceRefresh) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I read the code this does not only do "registration", but also auth token refresh.
Thus, I think the new name is misleading.
How about something that describes what is happening?
Here is a suggestion, maybe you have a better idea.

getLocalDataOrSendRequestToFirebaseInstallationsApi()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a related question: Where do you decide which request to actually send, i.e. #CreateInstallation ("registration") or #GenerateAuthToken ("refresh")?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the below method called doNetworkCallIfNecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it to doRegistrationOrRefresh.

@ankitaj224 ankitaj224 changed the title Revert "Completing getId call with the disk reads on the caller thread" Patch caching fid in memory on successful registration. Jul 8, 2020
@ankitaj224
Copy link
Contributor Author

As for the multi-threading with the tasks, executors, and listeners my understanding is limited, unfortunately. :/

No worries :) Fred reviewed those changes in another PR #1767

@ankitaj224 ankitaj224 merged commit 6f6006e into master Jul 9, 2020
@firebase firebase locked and limited conversation to collaborators Aug 8, 2020
@kaibolay kaibolay deleted the fisRevert branch September 14, 2022 17:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andirayo andirayo andirayo approved these changes

@ciarand ciarand Awaiting requested review from ciarand

@fredquintana fredquintana Awaiting requested review from fredquintana

Assignees
No one assigned
Labels
cla: yes Override cla size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ankitaj224 @google-oss-bot @andirayo @googlebot