Remove deprecated methods from RC. #2042

danasilver · 2020-10-06T16:45:58Z

Removes the following methods:

FirebaseRemoteConfig#activateFetched()
FirebaseRemoteConfig#getByteArray(String)
FirebaseRemoteConfig#setConfigSettings(FirebaseRemoteConfigSettings)
FirebaseRemoteConfig#setDefaults(Map<String,Object>)
FirebaseRemoteConfig#setDefaults(int)

FirebaseRemoteConfigSettings#isDeveloperModeEnabled()
FirebaseRemoteConfigSettings.Builder.setDeveloperModeEnabled(boolean)

and the FirebaseRemoteConfigFetchException class.

google-oss-bot · 2020-10-06T16:52:28Z

Coverage Report

Affected SDKs

`firebase-config`

SDK overall coverage changed from 88.72% (e65f487) to 89.02% (056025ca) by +0.30%.

Filename	Base (`e65f487`)	Head (056025ca)	Diff
ConfigCacheClient.java	93.55%	93.33%	-0.22%
ConfigFetchHandler.java	97.20%	97.18%	-0.02%
ConfigMetadataClient.java	89.47%	90.11%	+0.64%
FirebaseRemoteConfig.java	88.28%	89.19%	+0.91%
FirebaseRemoteConfigSettings.java	65.63%	61.54%	-4.09%

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 (056025ca) is created by Prow via merging commits: e65f487 102a838.

google-oss-bot · 2020-10-06T16:53:26Z

Binary Size Report

Affected SDKs

firebase-config

Type Base (e65f487) Head (056025ca) Diff

aar 193 kB 191 kB -2.23 kB (-1.2%)

apk (release) 1.02 MB 1.02 MB -480 B (-0.0%)
firebase-installations

Type Base (e65f487) Head (056025ca) Diff

aar 62.2 kB 62.2 kB +5 B (+0.0%)

Test Logs

Notes

Head commit (056025ca) is created by Prow via merging commits: e65f487 102a838.

erikeldridge

LGTM, but since we can't approve w changes requested, I'm selecting "request changes" just to highlight a question.

erikeldridge · 2020-10-07T19:18:23Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfig.java

-   *
-   * @param settings The new settings to be applied.
-   * @deprecated Use {@link #setConfigSettingsAsync(FirebaseRemoteConfigSettings)} instead.
-   */


Thinking (no action req'd): after we remove the sync methods, we'll be able to drop the "async" suffix and use the setConfigSettings signature for async 👍

We could, but that'd be a breaking API change, too.

I wonder if, instead of deleting this one, we could change it's return type to Task and deprecate the setConfigSettingsAsync() method. And then at some point delete the "Async" method. wdyt?

Would that be more confusing or error prone for developers on this update path? I'd figure many people use the deprecated sync methods and don't check the return type. Might create unexpected behavior to make them async with the existing name.

This PR currently removes the setConfigSettings method, so anyone using it is going to have to change code. The same would be true if we retained the method and made it async. Ultimately, I think it's relatively minor, since naming is hard :)

Yeah, my thought is that requiring the new method with Async in the name will prevent users from blindly switching to a method that's now async (when they previously might have fired and forgot, expecting a sync call).

erikeldridge · 2020-10-07T19:23:34Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfig.java

-   * Asynchronously sets the defaults cache to the given default values, and persists the values to
-   * disk.
-   */
-  private void setDefaultsWithStringsMap(Map<String, String> defaultsStringMap) {


Thinking (no action req'd): I wonder why a simple overload of setDefaults was insufficient and we needed to add the WithStringsMap qualifier

erikeldridge · 2020-10-07T19:27:44Z

.../main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigFetchThrottledException.java

@@ -15,8 +15,7 @@
 package com.google.firebase.remoteconfig;

 /** An exception thrown when a {@link FirebaseRemoteConfig#fetch()} call is throttled. */
-public class FirebaseRemoteConfigFetchThrottledException
-    extends FirebaseRemoteConfigFetchException {
+public class FirebaseRemoteConfigFetchThrottledException extends FirebaseRemoteConfigException {


Given the deprecation guidance on FirebaseRemoteConfigFetchException is to use FirebaseRemoteConfigServerException or FirebaseRemoteConfigClientException, I wonder if we should be deprecating FirebaseRemoteConfigFetchThrottledException. If so, we could make that a tiny follow-up change just to keep this PR focused.

The throttled exception makes more sense to me to keep since it has info (throttleEndTimeMillis) the super class doesn't.

erikeldridge · 2020-10-07T19:30:26Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigCacheClient.java

-   *
-   * @return A {@link Task} representing the write to disk.
-   */
-  public Task<ConfigContainer> putWithoutWaitingForDiskWrite(ConfigContainer configContainer) {


Thinking (no action req'd): I'm happy to see us reducing sources of ambiguity like this fire-and-forget async update

erikeldridge · 2020-10-07T20:11:54Z

.../bandwagoner/src/main/java/com/googletest/firebase/remoteconfig/bandwagoner/ApiFragment.java

-    frc.setConfigSettings(
-        new FirebaseRemoteConfigSettings.Builder().setDeveloperModeEnabled(true).build());
+    frc.setConfigSettingsAsync(
+        new FirebaseRemoteConfigSettings.Builder().setFetchTimeoutInSeconds(0L).build());


Dumb question: IIUC, this test app calls a live RC endpoint, in which case setting a timeout of 0s seems aggressive; any chance the intent is to bypass the cache (setMinimumFetchIntervalInSeconds ), which is closer to the functionality of developer mode, rather than setting a timeout?

Not a dumb question! That is actually what I intended to do, but I set the wrong setting! Thanks for questioning it. :P

danasilver added 3 commits October 6, 2020 12:43

Remove deprecated methods from RC.

ca25e51

Regenerate api.txt.

04a1bf1

Put back deprecation on test.

a381226

googlebot added the cla: yes Override cla label Oct 6, 2020

google-oss-bot added the size/XL label Oct 6, 2020

danasilver added 4 commits October 6, 2020 13:05

Update Bandwagoner to use new APIs.

38699c0

Use async method in tests.

32eb45a

googleJavaFormat

5fc3cbc

Fix up some tests.

69d0641

danasilver requested a review from erikeldridge October 7, 2020 16:07

erikeldridge suggested changes Oct 7, 2020

View reviewed changes

Set fetch interval instead of timeout in Bandwagoner.

102a838

erikeldridge approved these changes Oct 8, 2020

View reviewed changes

vkryachko approved these changes Oct 8, 2020

View reviewed changes

danasilver merged commit 83e9406 into master Oct 9, 2020

danasilver deleted the rc-remove-deprecated-apis branch October 9, 2020 14:50

firebase locked and limited conversation to collaborators Nov 9, 2020

Remove deprecated methods from RC. #2042

Remove deprecated methods from RC. #2042

Uh oh!

Conversation

danasilver commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Coverage Report

Affected SDKs

firebase-config

Test Logs

Notes

Uh oh!

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

Uh oh!

Binary Size Report

Affected SDKs

firebase-config

firebase-installations

Test Logs

Notes

Uh oh!

erikeldridge left a comment

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!

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!

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!

danasilver commented Oct 6, 2020 •

edited

Loading

google-oss-bot commented Oct 6, 2020 •

edited

Loading

`firebase-config`

google-oss-bot commented Oct 6, 2020 •

edited

Loading

`firebase-config`

`firebase-installations`