Skip to content
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.

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

Fireperf: create a random delay before initiating a remote config fetch. #2968

Merged
merged 8 commits into from
Sep 15, 2021

Conversation

leotianlizhan
Copy link
Contributor

Rationale:
This is required to ensure that a remote notification does not trigger multiple config fetches from remote config at the same time. Internal bug reference: b/187985523

iOS version of the same change firebase/firebase-ios-sdk#8593

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 10, 2021

Coverage Report

Affected SDKs

  • firebase-perf

    SDK overall coverage changed from 70.74% (b9f0ab6) to 70.79% (44dc407b) by +0.05%.

    Filename Base (b9f0ab6) Head (44dc407b) Diff
    RemoteConfigManager.java 92.24% 92.86% +0.62%

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 (44dc407b) is created by Prow via merging commits: b9f0ab6 b255861.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 10, 2021

Binary Size Report

Affected SDKs

  • firebase-perf

    Type Base (b9f0ab6) Head (44dc407b) Diff
    aar 299 kB 300 kB +463 B (+0.2%)
    apk (aggressive) 979 kB 980 kB +284 B (+0.0%)
    apk (release) 2.45 MB 2.45 MB +408 B (+0.0%)

Test Logs

Notes

Head commit (44dc407b) is created by Prow via merging commits: b9f0ab6 b255861.

@leotianlizhan
Copy link
Contributor Author

/retest

Copy link
Contributor

@visumickey visumickey left a comment

Choose a reason for hiding this comment

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

Thanks for making this change @leotianlizhan. Glad to see the changes. I have few comments. PTAL as it permits.

@leotianlizhan
Copy link
Contributor Author

/retest

Copy link
Contributor

@visumickey visumickey left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for making the changes Leo.

How are you planning to test this? It is important to do some manual testing to make sure we are able fetch the configs in less than 30 seconds and greater than 5 seconds. If you are able to ascertain that, please feel free to merge this change.

@leotianlizhan leotianlizhan force-pushed the fireperf-rc-fetch-delay branch from 2212970 to b255861 Compare September 15, 2021 15:20
@leotianlizhan
Copy link
Contributor Author

This is great! Thanks for making the changes Leo.

How are you planning to test this? It is important to do some manual testing to make sure we are able fetch the configs in less than 30 seconds and greater than 5 seconds. If you are able to ascertain that, please feel free to merge this change.

yes I've done some manual testing and it looks like it works

@leotianlizhan leotianlizhan merged commit 191700a into master Sep 15, 2021
@leotianlizhan leotianlizhan deleted the fireperf-rc-fetch-delay branch September 15, 2021 19:47
@firebase firebase locked and limited conversation to collaborators Oct 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
  翻译: