Enable request limiter for CreateInstallation, GenerateAuthToken request calls. #2003

ankitaj224 · 2020-09-23T16:43:13Z

Preventing erroneous requests.

request calls.

google-oss-bot · 2020-09-23T16:50:20Z

Binary Size Report

Affected SDKs

firebase-installations

Type Base (1443ac5) Head (2c8d6f45) Diff

aar 61.6 kB 62.2 kB +586 B (+1.0%)

apk (aggressive) 76.4 kB 76.7 kB +248 B (+0.3%)

apk (release) 648 kB 649 kB +280 B (+0.0%)

Test Logs

Notes

Head commit (2c8d6f45) is created by Prow via merging commits: 1443ac5 91ca982.

google-oss-bot · 2020-09-23T16:51:24Z

Coverage Report

Affected SDKs

`firebase-installations`

SDK overall coverage changed from 58.52% (1443ac5) to 58.74% (2c8d6f45) by +0.21%.

Filename	Base (`1443ac5`)	Head (2c8d6f45)	Diff
FirebaseInstallationServiceClient.java	5.05%	5.41%	+0.36%
FirebaseInstallationsException.java	76.92%	78.57%	+1.65%
Utils.java	93.75%	95.00%	+1.25%

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 (2c8d6f45) is created by Prow via merging commits: 1443ac5 91ca982.

andirayo · 2020-09-24T01:53:18Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

@@ -132,7 +132,7 @@ public Thread newThread(Runnable r) {
        new FirebaseInstallationServiceClient(
            firebaseApp.getApplicationContext(), publisher, heartbeatInfo),
        new PersistedInstallation(firebaseApp),
-        new Utils(),
+        Utils.getInstance(),
        new IidStore(firebaseApp),
        new RandomFidGenerator());


Why does RandomFidGenerator needs to be instantiated?
Usually I would assume, because a RandomNumberGenerator has to be injected, but that does not seem to be the case.
Thus, can we use the methods of this class statically?

As a more general note:
For general code design it would be better if there was a FIS model class that would represent FIDs and also carry methods like generating a random FID.
No action item! I just wanted to leave this comment here.

(fyi: this was not addressed or acknowledged)

Acknowledged. I will address this in a following PR.

firebase-installations/src/main/java/com/google/firebase/installations/Utils.java

andirayo · 2020-09-24T01:55:36Z

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

@@ -112,6 +115,7 @@ public FirebaseInstallationServiceClient(
    this.context = context;
    this.userAgentPublisher = publisher;
    this.heartbeatInfo = heartbeatInfo;
+    this.requestLimiter = new RequestLimiter(Utils.getInstance());


Is it necessary to pass the Utils parameter?
Since there is only one Utils instance, RequestLimiter should be able to access that instance itself and does not need to be passed a Utils instance.

Maybe the Utils parameter can be optional?
Not saying that it's better! Just thinking out loud! :)

Optional not supported due to minSDK version.
This is for testing this class easily.

By "optional" I mean that you have 2 constructors, one with Utils parameter, one without.
The constructor without Utils parameter just calls Utils.getInstance(), so we don't need this implementation detail here.

andirayo · 2020-09-24T02:32:58Z

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

@@ -143,7 +147,7 @@ public InstallationResponse createFirebaseInstallation(
    String resourceName = String.format(CREATE_REQUEST_RESOURCE_NAME_FORMAT, projectID);
    int retryCount = 0;
    URL url = getFullyQualifiedRequestUri(resourceName);
-    while (retryCount <= MAX_RETRIES) {
+    while (requestLimiter.isRequestAllowed() || shouldServerErrorRetry) {


Question:
When we should not send requests to the server, we throw a FirebaseInstallationsException with message "Firebase Installations Service is unavailable."? Is that correct and desired behavior?
If yes, why not throw this exception before the while loop?

Makes sense. Changed it to throw the exception before the loop.

Now about the desired behavior: We should throw an exception. But I am unclear if it should state "Firebase Installations Service is unavailable". WDYT?

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

andirayo · 2020-09-24T02:41:20Z

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

          retryCount++;
+          shouldServerErrorRetry = retryCount <= MAX_RETRIES;


I'm not sure if I like this code!
There is several things wrong with this I think:
a) When I read the While loop condition above, it is unclear to me what shouldServerErrorRetry is and I have to search the code to find where the variable is set.
b) If avoidable (which is almost always the case) local variables should not change in value, because it makes code hard to read - this is actually a great example why. If you have to do it at all costs, you should mark the local variable as @Var.
c) I just realize it's not a local variable, but an instance variable. Why is that? That is even more confusing to me. Are you saying that other parallel processes can change the value of the variable and affect this while loop? (c.1) I doubt that this is the case. (c.2) And if it's the case, I doubt that it's a good idea!
d) I also liked using a variable named retryCount in the while loop condition better than a variable called shouldServerErrorRetry... With the retryCount it is immediately obvious, that we will retry sending the same request for some times... shouldServerErrorRetry does not really give that much information.

I recommend:
Leave the retryCount <= MAX_RETRIES in the while loop.
Even better, get rid of the while loop and replace it with a for (int retryCount = 0; retryCount <= MAX_RETRIES; retryCount++).
That explains so much better what is happening here than a local variable that is changed somewhere else.

Changed it . PTAL.

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

...ase-installations/src/main/java/com/google/firebase/installations/remote/RequestLimiter.java

…o enableRequestLimiter

ankitaj224 · 2020-09-28T22:08:51Z

/test smoke-tests
/test device-check-changed

ankitaj224 · 2020-09-28T22:46:04Z

/test device-check-changed

…o enableRequestLimiter

ankitaj224 · 2020-09-29T21:45:02Z

/test smoke-tests

ankitaj224 · 2020-09-29T21:45:15Z

/test device-check-changed

andirayo · 2020-09-30T21:40:47Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

@@ -132,7 +132,7 @@ public Thread newThread(Runnable r) {
        new FirebaseInstallationServiceClient(
            firebaseApp.getApplicationContext(), publisher, heartbeatInfo),
        new PersistedInstallation(firebaseApp),
-        new Utils(),
+        Utils.getInstance(),
        new IidStore(firebaseApp),
        new RandomFidGenerator());


(fyi: this was not addressed or acknowledged)

firebase-installations/src/main/java/com/google/firebase/installations/Utils.java

andirayo · 2020-09-30T21:44:33Z

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

@@ -112,6 +115,7 @@ public FirebaseInstallationServiceClient(
    this.context = context;
    this.userAgentPublisher = publisher;
    this.heartbeatInfo = heartbeatInfo;
+    this.requestLimiter = new RequestLimiter(Utils.getInstance());


By "optional" I mean that you have 2 constructors, one with Utils parameter, one without.
The constructor without Utils parameter just calls Utils.getInstance(), so we don't need this implementation detail here.

…o enableRequestLimiter

google-oss-bot · 2020-10-05T18:16:53Z

@ankitaj224: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`91ca982`	link	`/test smoke-tests`

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. I understand the commands that are listed here.

Enable request limiter for CreateInstallation & GenerateAuthToken

f18cb66

request calls.

ankitaj224 requested a review from andirayo September 23, 2020 16:43

googlebot added the cla: yes Override cla label Sep 23, 2020

google-oss-bot added the size/M label Sep 23, 2020

andirayo reviewed Sep 24, 2020

View reviewed changes

ankitaj224 added 2 commits September 28, 2020 10:59

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

2033ccd

…o enableRequestLimiter

Addressing Rayo's comments.

6d6b7c3

google-oss-bot added size/L and removed size/M labels Sep 28, 2020

ankitaj224 requested a review from andirayo September 28, 2020 22:04

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

bb37ff1

…o enableRequestLimiter

andirayo approved these changes Sep 30, 2020

View reviewed changes

ankitaj224 added 3 commits October 2, 2020 11:23

Addressing rayo's comments

2266889

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

46726cb

…o enableRequestLimiter

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

91ca982

…o enableRequestLimiter

ankitaj224 merged commit 6d8b572 into master Oct 5, 2020

firebase locked and limited conversation to collaborators Nov 5, 2020

kaibolay deleted the enableRequestLimiter branch September 14, 2022 17:55

		retryCount++;
		shouldServerErrorRetry = retryCount <= MAX_RETRIES;

Enable request limiter for CreateInstallation, GenerateAuthToken request calls. #2003

Enable request limiter for CreateInstallation, GenerateAuthToken request calls. #2003

Uh oh!

Conversation

ankitaj224 commented Sep 23, 2020

Uh oh!

google-oss-bot commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-installations

Test Logs

Notes

Uh oh!

google-oss-bot commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-installations

Test Logs

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitaj224 commented Sep 28, 2020

Uh oh!

ankitaj224 commented Sep 28, 2020

Uh oh!

ankitaj224 commented Sep 29, 2020

Uh oh!

ankitaj224 commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Oct 5, 2020

Uh oh!

Uh oh!

google-oss-bot commented Sep 23, 2020 •

edited

Loading

`firebase-installations`

google-oss-bot commented Sep 23, 2020 •

edited

Loading

`firebase-installations`