Implemented exponential backoff and max retry with resumable uploads #4087

maneesht · 2022-09-14T14:44:29Z

We have exponential backoff implemented, but not for uploadChunk. If uploadChunk fails, we check if we are in a recoverable state, and then request the status of the upload, and then retry the upload immediately if the request was successful.

Instead of immediately retrying the upload, we should wait until the appropriate number of seconds, and then retry again. And we should only do so a max number of times. This PR implements both - exponential backoff and max retries.

google-oss-bot · 2022-09-14T14:50:12Z

Coverage Report ¹

Affected Products

`firebase-storage`

Overall coverage changed from ? (d1ea5b6) to 85.95% (c1db857) by ?.

46 individual files with coverage change

Filename	Base (`d1ea5b6`)	Merge (`c1db857`)	Diff
ActivityLifecycleListener.java	?	74.14%	?
AdaptiveStreamBuffer.java	?	84.62%	?
CancelException.java	?	100.00%	?
CancellableTask.java	?	100.00%	?
ControllableTask.java	?	100.00%	?
DeleteNetworkRequest.java	?	100.00%	?
DeleteStorageTask.java	?	100.00%	?
ExponentialBackoffSender.java	?	86.00%	?
FileDownloadTask.java	?	80.00%	?
FirebaseStorage.java	?	83.67%	?
FirebaseStorageComponent.java	?	100.00%	?
GetDownloadUrlTask.java	?	96.77%	?
GetMetadataNetworkRequest.java	?	100.00%	?
GetMetadataTask.java	?	85.19%	?
GetNetworkRequest.java	?	100.00%	?
HttpURLConnectionFactory.java	?	0.00%	?
HttpURLConnectionFactoryImpl.java	?	50.00%	?
ListNetworkRequest.java	?	100.00%	?
ListResult.java	?	100.00%	?
ListTask.java	?	85.71%	?
NetworkRequest.java	?	86.67%	?
OnPausedListener.java	?	0.00%	?
OnProgressListener.java	?	0.00%	?
ResumableNetworkRequest.java	?	100.00%	?
ResumableUploadByteRequest.java	?	90.91%	?
ResumableUploadCancelRequest.java	?	100.00%	?
ResumableUploadQueryRequest.java	?	100.00%	?
ResumableUploadStartRequest.java	?	95.24%	?
Slashes.java	?	88.24%	?
Sleeper.java	?	0.00%	?
SleeperImpl.java	?	100.00%	?
SmartHandler.java	?	87.50%	?
StorageException.java	?	69.09%	?
StorageMetadata.java	?	86.34%	?
StorageReference.java	?	89.94%	?
StorageReferenceUri.java	?	100.00%	?
StorageRegistrar.java	?	100.00%	?
StorageTask.java	?	84.59%	?
StorageTaskManager.java	?	100.00%	?
StorageTaskScheduler.java	?	100.00%	?
StreamDownloadTask.java	?	88.89%	?
TaskListenerImpl.java	?	100.00%	?
UpdateMetadataNetworkRequest.java	?	100.00%	?
UpdateMetadataTask.java	?	82.14%	?
UploadTask.java	?	80.46%	?
Util.java	?	73.24%	?

Test Logs

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

github-actions · 2022-09-14T14:55:51Z

Unit Test Results

  27 files -   366   27 suites - 366 3m 59s ⏱️ - 13m 58s
168 tests - 4 544 168 ✔️ - 4 521 0 💤 - 22 0 ❌ - 1
168 runs - 4 560 168 ✔️ - 4 537 0 💤 - 22 0 ❌ - 1

Results for commit 9ca073b. ± Comparison against base commit 6caf590.

♻️ This comment has been updated with latest results.

google-oss-bot · 2022-09-14T14:59:22Z

Size Report ¹

Affected Products

firebase-storage
Type Base (d1ea5b6) Merge (c1db857) Diff
aar 115 kB 116 kB +968 B (+0.8%)
apk (release) 1.01 MB 1.01 MB +468 B (+0.0%)

Test Logs

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

…backoff-resumableupload

maneesht · 2022-09-22T17:45:34Z

firebase-storage/src/test/java/com/google/firebase/storage/TestUtil.java

+    // With the 5 second keepalive, after 5 seconds, the thread will get killed and eventually a new
+    // one will be created.
+    // Therefore causing many of the tests to fail
+    StorageTaskScheduler.setCallbackQueueKeepAlive(90, TimeUnit.SECONDS);


To add additional context:

We call attachListeners https://github.com/firebase/firebase-android-sdk/blob/ed10eb5ebe998dc23ead46419a2b4dcbb5e2482c/firebase-storage/src/testUtil/java/com/google/firebase/storage/TestUploadHelper.java#L49 to set up the listeners and then those listeners call ControllableSchedulerHelper to verify the callback thread that it initially was called with is the same one that is called in subsequent events

firebase-android-sdk/firebase-storage/src/testUtil/java/com/google/firebase/storage/ControllableSchedulerHelper.java

Line 63 in ed10eb5

public void verifyCallbackThread() {

However, in cases where we have long backoffs, this won't be the case, as the thread pool will terminate any threads that are not executed within the keepalive time, which is 5 seconds.
Our alternative is to set the keepAlive, or disable the threads from being killed. A third option would be to remove this verification check.

Do we know why this check is necessary? I'm not familiar with the tradeoffs of keeping threads around for an extended period of time.

I think in case the wrong executor gets called by UploadTask.

Here's a quick StackOverflow answer that talks about the tradeoffs:
https://stackoverflow.com/a/18225863

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

tonyjhuang

small comments, overall looks good!

tonyjhuang · 2022-09-22T17:52:59Z

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

+  /*package*/ static Sleeper sleeper = new SleeperImpl();
+  /*package*/ static Clock clock = DefaultClock.getInstance();
+  private int sleepTime =
+      0; // TODO(mtewani): Make it so that the send is 0,1,2,4,8,... and start at 0


remove comment?

tonyjhuang · 2022-09-22T17:53:27Z

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

+  /*package*/ static Clock clock = DefaultClock.getInstance();
+  private int sleepTime =
+      0; // TODO(mtewani): Make it so that the send is 0,1,2,4,8,... and start at 0
+  private final int sleepInterval = 1000;


So this is essentially a minimum sleep time for retries right?

consider renaming to minimumSleepInterval?

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

tonyjhuang · 2022-09-22T17:59:57Z

firebase-storage/src/test/java/com/google/firebase/storage/TestUtil.java

@@ -32,6 +32,12 @@
 public class TestUtil {

  static FirebaseApp createApp() {
+    // Many tests require you to call the callback on the same thread that was initially
+    // instantiated.


comment formatting

firebase-storage/src/test/java/com/google/firebase/storage/TestUtil.java

tonyjhuang · 2022-09-22T18:02:15Z

firebase-storage/src/test/java/com/google/firebase/storage/UploadTest.java

@@ -130,7 +160,7 @@ public void cantUploadToRoot() throws Exception {
        });

    // TODO(mrschmidt): Lower the timeout
-    TestUtil.await(task, 300, TimeUnit.SECONDS);
+    TestUtil.await(task, 7, TimeUnit.MINUTES);


this seems at odds with the line comment above😅 why is this necessary?

Ended up changing this to 1 minute. The 7 was a precaution :-)

tonyjhuang · 2022-09-23T15:18:00Z

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java

      }
-      return false;
+      sleepTime = Math.max(sleepTime * 2, sleepTime + (minimumSleepInterval * 2));


you probably want

Math.max(sleepTime * 2, minimumSleepInterval)

The way this is written, the first sleep time == 2000

Oh wow. Nice catch! Will change

* Deflake firebase_common HeartBeat tests. (#4083) The tests relied on `TestOnCompleteListener` that was not safe to call more than once since it was based on a count down latch. So reusing it multiple times would cause await() to return immediately. This change makes it so that a new latch is created for every await() call, making all await() calls work. Fixes: http://b/245956774 * Add Javadoc support to the DackkaPlugin (#4082) * Add util method for copying directories * Add javadoc support to our dackka plugin * Remove the extension check on fromDirectory * Add a note about cache compliance and the javadoc task * Add reference to kotlin stdlib package list (#4093) * Add appcheck-ktx to bom config (#4081) * Removing getRunningAppProcesses since the process_name isn't used (#4057) * Fix Documentation classpath (#4099) b/241795594 * Add projectSpecificSources back to the DackkaPlugin (#4110) * Added extra method for TaskProviders * Added specificSources method back * Revert to dependsOn for docstubs dep * Revamp test harness for macrobenchmark tests (#4071) * Fix dependabot security alerts (#4123) * Make firesite transform cacheable (#4124) * add coroutines-play-services as a transitive dep to firebase-common-ktx (#4044) * add kotlinx-coroutines-play-services as a transitive dep to firebase-common-ktx * Update to Coroutines 1.6.4 * database-ktx: add callbackFlow for eventlisteners (#4012) * add callbackFlow for RTDB's ValueEventListener * add callbackFlow for RTDB's ChildEventListener * delegate trySendBlocking to DefaultRunLoop * add group to ktx.gradle * update api.txt file * Update released versions (#4135) * Upgrade dackkaConfig (#4141) * Add names to all Firebase components (#4117) * Add appcheck's ktx artifact back to package list file (#4142) * Add strict mode testing in firebase-messaging (#4095) * Add gralde property to instrument Fireperf E2E test (#4144) The perf gradle PR is #334 in the gradle repo. b/246802885 * Resolve StrictMode violation in App Check. (#4085) * Resolve StrictMode violation in App Check. * Attempt to fix some tests. * Fix unit tests. * Make `retrieveStoredAppCheckTokenInBackground` private instead of package-private. * Move listener invocations back to the main thread while keeping disk write on background thread. * Refactor to use lambda syntax. * Implemented exponential backoff and max retry with resumable uploads (#4087) * storage-ktx: add callbackFlow for upload/download progress (#4139) * add kotlin flows to storage * update api.txt file * add group to storage/ktx.gradle * Make a best effort attempt to flush reports at crash time (#4112) This should allow us to upload reports for start-up crashes. * Public Count (#4130) * Public Count * Disable prod testing * Long to long * Api.txt * Backfill changelog * Add PR * Fix assertEquals error * Re-write API javadocs for COUNT API (#4143) Co-authored-by: Denver Coneybeare <[email protected]> * [Fireperf][AASA] send `_experiment_app_start_ttid` trace, controlled by RC flag (#4114) * log _experiment_as_ttid * send event and RRC mitigation * add RC wip * modified save to cache when RC fetches * dev-app manifest override * unit test for RCc cache saving * better name and comments * better formatting remoteconfigmanagertest * better comments and added local RC lookup back * Specify unique ref tags in Dackka output (#4149) * Add util methods for gradle projects * Disable Javadoc generation on empty projects * Fix ref path generation in Dackka output * Add documentation for util methods * Update the DackkaPlugin docs * Reduced path to relative from tenant * Reduced ref head path even more * Fixed ref tag path to working solution * Disabled publishJavadoc by default for tests It should be enabled explicitly when being tested anyhow, and causes issues otherwise. Instead of disabling it for the tests that don't need it- this is much quicker and easier to manager. * Add strict mode tests to inappmessaging and inappmessaging-display (#4136) * Fix strict mode violations for appcheck (#4148) * Fix strict mode violations for appcheck * Formatting * Add copyright header * Populate SDKs changelog files (#4070) * first try seeding changelogs * Added unreleased section to CHANGELOG * Fix empty lines between sections * Add missing entries for abt * Update data to include latest releases * Update CHANGELOG.md * Add missing line in unreleased section for perf. * Enable CHANGELOG check globally (#4084) * Enable CHANGELOG check globally * Simplify conditional. * Enable COUNT integration tests, now that backend support has rolled out (#4163) * Remove separation of kotlin directories in dackka (#4166) * Deprecate App Check SafetyNet SDK (#4187) * Add `@Deprecated` annotations to Firebase App Check SafetyNet SDK public API. * Add `@deprecated` tag in the Javadoc as well. * Remove stale entries from Unreleased section. (#4185) * Assign ConfigContainer Builder return values. (#4194) * update bom (#4155) * update bom * update * update * feat(perf-ktx): add trace(name, block) extension function (#4180) * Remove smoke test for app indexing (#4219) App Indexing is deprecated starting BoM 31.0.0 . https://firebase.google.com/support/release-notes/android#bom_v31-0-0 * Bump Robolectric to 4.9 (#4161) * Add plexus-utils for firebase-database tests Looks like firebase-database tests use plexus-utils dependency of Robolectric directly. But this dependency was removed by Robolectric. So this CL adds plexus-utils explictly for firebase-database tests. Signed-off-by: utzcoz <[email protected]> * Bump Robolectric to 4.9 1. Use legacy LooperMode for tests explicitly, because recent Robolectric releases switch to use PAUSED mode default. Before these tests migrate to PAUSED mode, they use LEGACY mode to pass tests. 2. Migrate Assert.assertThat to Truth.assertThat to avoid using removed APIs. 3. All build.gradle use the same robolectricVersion except transport-backend-cct because Robolectric 4.8+ has compatibility problem for TelephonyManager with low compile/targetSdkversion. To keep httpclient compatibility, transport-backend-ccts continues to use Robolectric 4.3.1. 4. Remove unused exclude protobuf-java from Robolectric. 5. Add necessary protobuf-lite dependency on classpath for some ktx modules' tests. Signed-off-by: utzcoz <[email protected]> Signed-off-by: utzcoz <[email protected]> * return exception if modelname is empty (#4226) * Add "create release PR" github action (#4236) This implementation: - Creates the base branch (name is based in user input) - Creates the release branch (name is based in user input) - Creates the release.cfg file in the release branch without adding any SDK (module) to it. It can create the branches based on any existing branch of the repo. * Sync spec tests from web SDK to Android SDK (#4230) * Update versions (#4238) * Update versions * Exclude .github dir from `firebaseContinuousIntegration` paths (#4239) * Performing IN expansion (#4221) * WIP: `in` expansion. * Add composite filter in-expansion test. * Fix formatting. * Run in-expansion as part of DNF computation and add tests. * Add test with nested IN filters with CSI. * Add tests for other cases. * typo fix (#4237) * Firestore: Add test that verifies count query error message when missing index (#4232) * refactor(functions): update firebase-iid to 21.1.0 (#4225) * refactor(functions): update firebase-iid to 21.1.0 * Update CHANGELOG.md * Update CHANGELOG.md * bump firebase-iid-interop to 17.1.0 * exclude firebase-components from firebase-iid dependency Signed-off-by: utzcoz <[email protected]> Co-authored-by: Vladimir Kryachko <[email protected]> Co-authored-by: Daymon <[email protected]> Co-authored-by: Raymond Lam <[email protected]> Co-authored-by: Yifan Yang <[email protected]> Co-authored-by: Rosário Pereira Fernandes <[email protected]> Co-authored-by: emilypgoogle <[email protected]> Co-authored-by: Jeremy Jiang <[email protected]> Co-authored-by: Rosalyn Tan <[email protected]> Co-authored-by: Maneesh Tewani <[email protected]> Co-authored-by: Matthew Robertson <[email protected]> Co-authored-by: wu-hui <[email protected]> Co-authored-by: Denver Coneybeare <[email protected]> Co-authored-by: Leo Zhan <[email protected]> Co-authored-by: Rodrigo Lazo <[email protected]> Co-authored-by: Dana Silver <[email protected]> Co-authored-by: Vinay Guthal <[email protected]> Co-authored-by: utzcoz <[email protected]> Co-authored-by: argzdev <[email protected]> Co-authored-by: Mila <[email protected]> Co-authored-by: Ehsan <[email protected]> Co-authored-by: cherylEnkidu <[email protected]>

Implemented exponential backoff and max retry with resumable uploads

dd3fba3

google-oss-bot added the size/S label Sep 14, 2022

maneesht marked this pull request as draft September 14, 2022 14:50

maneesht added 2 commits September 14, 2022 13:02

Merge remote-tracking branch 'origin/master' into mtewani/exponential…

08c5714

…backoff-resumableupload

Added test for exponential backoff/retry

16cffe8

google-oss-bot added size/XXL and removed size/S labels Sep 21, 2022

Updated formatting

10308ff

google-oss-bot added size/XL and removed size/XXL labels Sep 21, 2022

maneesht added 7 commits September 21, 2022 12:42

Updated timeout and allowed for configurability

7a9356c

Moved backoff to uploadChunks

a47f2dc

Removed delaySend from public api

d24c56d

Fixed formatting

11f5f54

Refactored to use existing fileupload

fdb61c9

Merge branch 'master' into mtewani/exponentialbackoff-resumableupload

ad8203e

Updated tests

6acea85

google-oss-bot added size/XXL and removed size/XL labels Sep 22, 2022

maneesht added 4 commits September 22, 2022 07:02

Updated tests

ea6d56a

Removed print

c6ac2f1

Updated changelog

1384235

Updated formatting and decreased the keepalive timer

0bf2349

maneesht commented Sep 22, 2022

View reviewed changes

maneesht marked this pull request as ready for review September 22, 2022 17:45

maneesht requested a review from tonyjhuang September 22, 2022 17:45

maneesht commented Sep 22, 2022

View reviewed changes

firebase-storage/src/main/java/com/google/firebase/storage/UploadTask.java Show resolved Hide resolved

tonyjhuang reviewed Sep 22, 2022

View reviewed changes

Addressed comments

047a780

google-oss-bot added size/XL and removed size/XXL labels Sep 22, 2022

maneesht added 8 commits September 22, 2022 11:44

Addressed comments

da6b659

Removed unnecessary line

1304294

Addressed comment

7f8772b

Replaced retryrule temporarily

91c5403

Increase timeout for fileUploadRecovery

ce95d65

Increase timeout for fileUploadRecovery

277b03e

Updated with logging info

fc9c9ac

Removed logging

d37c628

tonyjhuang reviewed Sep 23, 2022

View reviewed changes

tonyjhuang approved these changes Sep 23, 2022

View reviewed changes

Addressed comments

7787770

google-oss-bot added size/L and removed size/XL labels Sep 23, 2022

Updated text files properly

254d8cc

google-oss-bot added size/XL and removed size/L labels Sep 23, 2022

maneesht added 5 commits September 23, 2022 09:04

Updated timeout for recovery test

1085b00

Reduced retry count again

97520ea

Updated to increase timeout

91a72a5

Updated uploadtask

d6da159

Removed unnecessary logs

9ca073b

maneesht merged commit 988ff71 into master Sep 26, 2022

maneesht deleted the mtewani/exponentialbackoff-resumableupload branch September 26, 2022 20:47

firebase locked and limited conversation to collaborators Oct 27, 2022

Implemented exponential backoff and max retry with resumable uploads #4087

Implemented exponential backoff and max retry with resumable uploads #4087

Uh oh!

Conversation

maneesht commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Coverage Report 1

Affected Products

firebase-storage

Test Logs

Uh oh!

github-actions bot commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

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

Uh oh!

Size Report 1

Affected Products

firebase-storage

Test Logs

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!

tonyjhuang 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maneesht commented Sep 14, 2022 •

edited

Loading

google-oss-bot commented Sep 14, 2022 •

edited

Loading

Coverage Report ¹

`firebase-storage`

github-actions bot commented Sep 14, 2022 •

edited

Loading

google-oss-bot commented Sep 14, 2022 •

edited

Loading

Size Report ¹

`firebase-storage`