Skip to content

Lazy initialize dataFile in PersistedInstallation #3314

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 1 commit into from
Jan 25, 2022
Merged

Lazy initialize dataFile in PersistedInstallation #3314

merged 1 commit into from
Jan 25, 2022

Conversation

carlonzo
Copy link
Contributor

tries to solve #3313

the dataFile is initialized lazily from methods that are supposed to be already be called from a dedicated IO thread given they perform disk ops

@google-oss-bot
Copy link
Contributor

Hi @carlonzo. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@carlonzo carlonzo force-pushed the lazy-init-datafile-persistedintallation branch from 1852750 to e58be5f Compare January 14, 2022 08:33
@yoyomyo yoyomyo assigned yoyomyo and unassigned yuchenshi Jan 18, 2022
@yoyomyo
Copy link

yoyomyo commented Jan 20, 2022

Very nice solution. Thank you, Carlo!
/ok-to-test

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 20, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-installations

    Overall coverage changed from ? (f620db9) to 60.21% (0046f17) by ?.

    22 individual files with coverage change

    FilenameBase (f620db9)Merge (0046f17)Diff
    AutoValue_InstallationResponse.java?58.93%?
    AutoValue_PersistedInstallationEntry.java?61.18%?
    AutoValue_TokenResult.java?45.65%?
    AwaitListener.java?0.00%?
    Clock.java?0.00%?
    CrossProcessLock.java?51.85%?
    FirebaseInstallations.java?98.04%?
    FirebaseInstallationServiceClient.java?5.26%?
    FirebaseInstallationsException.java?78.57%?
    FirebaseInstallationsRegistrar.java?100.00%?
    GetAuthTokenListener.java?100.00%?
    GetIdListener.java?70.00%?
    IidStore.java?44.78%?
    InstallationResponse.java?100.00%?
    PersistedInstallation.java?97.01%?
    PersistedInstallationEntry.java?100.00%?
    RandomFidGenerator.java?20.00%?
    RequestLimiter.java?100.00%?
    StateListener.java?0.00%?
    SystemClock.java?75.00%?
    TokenResult.java?100.00%?
    Utils.java?95.00%?

Test Logs

Notes

  • Commit (0046f17) is created by Prow via merging PR base commit (f620db9) and head commit (77ed36f).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/aNY2QJjcnk.html

@carlonzo carlonzo force-pushed the lazy-init-datafile-persistedintallation branch from e58be5f to 88aefec Compare January 21, 2022 06:38
@carlonzo
Copy link
Contributor Author

thanks! please

/retest

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 21, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-installations

    TypeBase (f620db9)Merge (0046f17)Diff
    aar54.6 kB54.7 kB+99 B (+0.2%)
    apk (aggressive)79.5 kB79.3 kB-232 B (-0.3%)

Test Logs

Notes

  • Commit (0046f17) is created by Prow via merging PR base commit (f620db9) and head commit (77ed36f).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/4rKs0B8lZZ.html

@carlonzo carlonzo force-pushed the lazy-init-datafile-persistedintallation branch from 88aefec to f06677c Compare January 21, 2022 08:02
@carlonzo
Copy link
Contributor Author

hi @yoyomyo,
the test device-check-changed fails due to the failure of :firebase-crashlytics:firebase-test-labUpload but I cant find the reason this is related to my PR. also logs https://storage.googleapis.com/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3314/device-check-changed/1484436446457630724/build-log.txt do not give me any hint on whats happening. you mind giving me an hint?

@yoyomyo
Copy link

yoyomyo commented Jan 24, 2022

@carlonzo Thank you for following up! The test failure does not seem related to this PR. I have reached out to the wider SDK team for support since I am not super familiar with the firebase-crashlytics:firebase-test-labUpload error. And I hope you don't mind me rerunning the test just to see what happens.
/retest

@carlonzo
Copy link
Contributor Author

@yoyomyo ofc thanks. Please let me know if there is anything I can do to help

@rlazo
Copy link
Collaborator

rlazo commented Jan 24, 2022

/test device-check-changed

@yoyomyo yoyomyo self-requested a review January 24, 2022 21:25
@yoyomyo
Copy link

yoyomyo commented Jan 24, 2022 •
edited
Loading

@carlonzo apologies about the inconvenience. We submitted a fix for the test breakage. Please try to sync and run the failed test again. Thank you!

@carlonzo carlonzo force-pushed the lazy-init-datafile-persistedintallation branch from f06677c to 77ed36f Compare January 25, 2022 05:43
@rlazo rlazo merged commit b7c3081 into firebase:master Jan 25, 2022
@rlazo
Copy link
Collaborator

rlazo commented Jan 25, 2022

Thanks for your contribution @carlonzo !

@carlonzo carlonzo deleted the lazy-init-datafile-persistedintallation branch January 25, 2022 14:24
@carlonzo
Copy link
Contributor Author

no prob @rlazo @yoyomyo thanks for your quick feedbacks. now that I know you guys are nice, I'll send more 😄

@carlonzo carlonzo mentioned this pull request Jan 25, 2022
Closed
@firebase firebase locked and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yoyomyo yoyomyo yoyomyo approved these changes

Assignees

@yoyomyo yoyomyo

Labels
ok-to-test size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@carlonzo @google-oss-bot @yoyomyo @rlazo @yuchenshi