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.

Sign up for GitHub

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

fix: Always set the retry attempt to 0 for now #1251

Merged
merged 5 commits into from
Mar 15, 2023
Merged

fix: Always set the retry attempt to 0 for now #1251

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d46a05
Always set the retry attempt to 0 for now
danieljbruce Mar 13, 2023
8157de4
fix: Always set the retry attempt to 0 for now
danieljbruce Mar 13, 2023
a2a7766
Merge branch 'set-number-of-consecutive-errors-to-0' of https://githu…
danieljbruce Mar 13, 2023
9ec6a80
Merge branch 'main' into set-number-of-consecutive-errors-to-0
danieljbruce Mar 13, 2023
5cfd334
Merge branch 'main' into set-number-of-consecutive-errors-to-0
danieljbruce Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
} as google.bigtable.v2.IReadRowsRequest;

const retryOpts = {
currentRetryAttempt: numConsecutiveErrors,
currentRetryAttempt: 0, // was numConsecutiveErrors
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why "numConsecutiveErrors" doesn't work here even though its value is 0 according to Line 730?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

numConsecutiveErrors works, but if it is greater than 0 then that will make the request take longer. Note that the number of consecutive errors is incremented at

nodejs-bigtable/src/table.ts

Line 924 in 44fab5c

numConsecutiveErrors++;
on each retry.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this

calculatedNextRetryDelay = config.initialRetryDelayMillis * Math.pow(config.retryDelayMultiplier, numConsecutiveErrors)

It implies that when it's executed for the first time (i.e. the original request has failure), numConsecutiveErrors should be 0 such that the retry delay is config.initialRetryDelayMillis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that the change in the PR doesn't affect this code. This is a delay that happens in the veneer layer while the delay in retryRequest happens below the veneer layer.

// Handling retries in this client. Specify the retry options to
// make sure nothing is retried in retry-request.
noResponseRetries: 0,
Expand Down