feat: customize retry behavior implementation #1474

shaffeeullah · 2021-05-27T15:10:59Z

Custom retry strategy behavior on the storage side. Will merge all storage retry PRs into the shaffeeullah/retryStrategy branch and then merge that branch into master at the end. This avoids one giant PR.

Note: autoRetry and maxRetries implementation currently relies on nodejs-common defaults. This PR also pulls control over these defaults into the nodejs-storage library.

…is/nodejs-storage into shaffeeullah/retryConfiguration

tritone · 2021-06-01T15:40:14Z

src/storage.ts

@@ -371,6 +374,13 @@ export class Storage extends Service {
   * @property {boolean} [autoRetry=true] Automatically retry requests if the
   *     response is related to rate limits or certain intermittent server
   * errors. We will exponentially backoff subsequent requests by default.
+   * @property {number} [backoffMultiplier = 2] Dictates the rate of exponential backoff.


I think it might be clearer if you add a little explanation/example about how these parameters interact with one another. For example with the default settings, up to how many retries will happen and how much will they be spaced out? You could also link to something explaining backoff if that's easier.

src/storage.ts

tritone · 2021-06-01T15:43:44Z

src/storage.ts

@@ -46,6 +46,9 @@ export interface CreateBucketQuery {
 }

 export interface StorageOptions extends ServiceOptions {
+  backoffMultiplier?: number;


Have we considered factoring the retry options into some sort of sub-config? Not sure if this is a thing in Node. In Go for example the various mathy configs are all in https://pkg.go.dev/github.com/googleapis/gax-go/v2#Backoff

tritone · 2021-06-01T17:30:11Z

src/storage.ts

@@ -393,6 +405,12 @@ export class Storage extends Service {
   * @param {StorageOptions} [options] Configuration options.
   */
  constructor(options: StorageOptions = {}) {
+    const AUTO_RETRY_DEFAULT = true;
+    const MAX_RETRY_DEFAULT = 3;


Was this constant being used already? 3 seems way too low.

Yeah this is the current default

…dejs-storage into shaffeeullah/retryConfiguration

…is/nodejs-storage into shaffeeullah/retryConfiguration

ddelgrosso1 · 2021-06-16T16:36:54Z

src/storage.ts

@@ -393,6 +420,12 @@ export class Storage extends Service {
   * @param {StorageOptions} [options] Configuration options.
   */
  constructor(options: StorageOptions = {}) {
+    const AUTO_RETRY_DEFAULT = true;


Would it make sense to move these constants to the class level along with a comment for consistency with other files? For example

nodejs-storage/src/bucket.ts

Line 381 in fbb90c2

const RESUMABLE_THRESHOLD = 5000000;

fixed! good callout

…is/nodejs-storage into shaffeeullah/retryConfiguration

ddelgrosso1 · 2021-06-16T17:36:08Z

test/index.ts

+    it('should throw if autoRetry is defined twice', () => {
+      const autoRetry = 10;
+      assert.throws(() => {
+        const storage = new Storage({


Since this is testing that the constructor throws an exception could the assignment to the const be removed to cleanup the linter warning? Same goes for L285.

whoops, totally missed the linter warnings. fixed

…is/nodejs-storage into shaffeeullah/retryConfiguration

ddelgrosso1

LGTM

* feat: customize retry behavior implementation (#1474) * feat: customize retry behavior implementation * 🦉 Updates from OwlBot * fixed != * 🦉 Updates from OwlBot * updated names to match gogle gax * 🦉 Updates from OwlBot * refactored retryOptions into its own config * 🦉 Updates from OwlBot * fixed linting error * 🦉 Updates from OwlBot * added retry delay explanation * 🦉 Updates from OwlBot * refactored constants * 🦉 Updates from OwlBot * removed const assignment Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * feat: implemented retry function (#1476) * implemented retry function * 🦉 Updates from OwlBot * fixed import * 🦉 Updates from OwlBot * resolved merge conflict * passed retry function to common * 🦉 Updates from OwlBot * refactored code to retryableErrFn * removed unused import * fixed typo * fixed typo * fixed failing tests * made retryableErrorFn configurable * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * wrote unit tests * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * changed reason check to be less brittle * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * depend on common 3.7.0 for retry changes * feat: remove gaxios dependency (#1503) * feat: remove gaxios dependency * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * moved callbackFunction inline * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * assigned types to any * changed any to string Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * feat: applied customization to multipart filesave (#1504) * feat: applied customization to multipart filesave * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * fixed failing tests * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * removed unused import Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * Update storage.ts added comment next to EAI_AGAIN * feat: pass retryOptions to gcs-resumable-upload (#1506) * feat: pass retryOptions to gcs-resumable-upload * linter fixes * additional test assertions for retryOptions * add additional asserts to resumable operation tests Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Denis DelGrosso <[email protected]>

feat: customize retry behavior implementation

6b5641d

shaffeeullah requested a review from a team May 27, 2021 15:10

shaffeeullah requested a review from a team as a code owner May 27, 2021 15:10

product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label May 27, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2021

shaffeeullah requested review from danielduhh and frankyn May 27, 2021 15:11

gcf-owl-bot bot and others added 4 commits May 27, 2021 15:12

🦉 Updates from OwlBot

5a9eda8

fixed !=

411c80e

Merge branch 'shaffeeullah/retryConfiguration' of github.com:googleap…

e856973

…is/nodejs-storage into shaffeeullah/retryConfiguration

🦉 Updates from OwlBot

293399b

tritone reviewed Jun 1, 2021

View reviewed changes

shaffeeullah and others added 2 commits June 1, 2021 16:33

updated names to match gogle gax

507edb3

🦉 Updates from OwlBot

3d0c06f

tritone reviewed Jun 1, 2021

View reviewed changes

shaffeeullah and others added 8 commits June 15, 2021 21:56

Merge branch 'shaffeeullah/retryStrategy' of github.com:googleapis/no…

be557ca

…dejs-storage into shaffeeullah/retryConfiguration

refactored retryOptions into its own config

7c0bc7f

🦉 Updates from OwlBot

3b729ec

fixed linting error

15a4462

🦉 Updates from OwlBot

0f85075

added retry delay explanation

e53a528

Merge branch 'shaffeeullah/retryConfiguration' of github.com:googleap…

5bf7e1d

…is/nodejs-storage into shaffeeullah/retryConfiguration

🦉 Updates from OwlBot

cbf137f

shaffeeullah requested a review from ddelgrosso1 June 16, 2021 15:38

ddelgrosso1 reviewed Jun 16, 2021

View reviewed changes

shaffeeullah and others added 3 commits June 16, 2021 17:08

refactored constants

2a41852

Merge branch 'shaffeeullah/retryConfiguration' of github.com:googleap…

22e1e9f

…is/nodejs-storage into shaffeeullah/retryConfiguration

🦉 Updates from OwlBot

3404c38

ddelgrosso1 reviewed Jun 16, 2021

View reviewed changes

removed const assignment

9a6bb9f

Merge branch 'shaffeeullah/retryConfiguration' of github.com:googleap…

db23721

…is/nodejs-storage into shaffeeullah/retryConfiguration

ddelgrosso1 approved these changes Jun 16, 2021

View reviewed changes

shaffeeullah merged commit cb0d9ac into shaffeeullah/retryStrategy Jun 16, 2021

shaffeeullah deleted the shaffeeullah/retryConfiguration branch June 16, 2021 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: customize retry behavior implementation #1474

feat: customize retry behavior implementation #1474

shaffeeullah commented May 27, 2021

tritone Jun 1, 2021

tritone Jun 1, 2021

tritone Jun 1, 2021

shaffeeullah Jun 1, 2021

ddelgrosso1 Jun 16, 2021

shaffeeullah Jun 16, 2021

ddelgrosso1 Jun 16, 2021

shaffeeullah Jun 16, 2021

ddelgrosso1 left a comment

feat: customize retry behavior implementation #1474

feat: customize retry behavior implementation #1474

Conversation

shaffeeullah commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddelgrosso1 left a comment

Choose a reason for hiding this comment