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.

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: correctly calculate retry attempt #33

Merged
merged 3 commits into from Jul 31, 2020
Merged

Conversation

stephenplusplus
Copy link
Contributor

The code was confusing "retry attempts" with "request attempts". This sets the record straight.

@@ -272,7 +272,7 @@ describe('retry-request', function () {
};

retryRequest(URI_404, opts, function (err) {
assert.strictEqual(numAttempts, 2);
assert.strictEqual(numAttempts, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default number of retries is 2, which means we should make a total of 3 requests for the user who doesn't have any custom settings. In this case, the test says "we've already made 1 retry" by setting "currentRetryAttempt" to 1. That means there's one more retry to go until we reach 2 retries. So, total number of requests = 1.

'currentRetryAttempt` was introduced exclusively for Bigtable: googleapis/nodejs-bigtable#27. I wasn't the author of that code, but I ran this change against the system tests, and they still pass.

@stephenplusplus stephenplusplus merged commit 4c852e2 into master Jul 31, 2020
@stephenplusplus stephenplusplus deleted the spp--retry-attempt branch July 31, 2020 17:09
gcf-merge-on-green bot pushed a commit that referenced this pull request Apr 11, 2022
🤖 I have created a release *beep* *boop*
---


## [4.2.0](v4.1.0...v4.2.0) (2022-04-06)


### Features

* support enhanced retry settings ([#35](#35)) ([0184676](0184676))


### Bug Fixes

* add new retry options to types ([#36](#36)) ([3f10798](3f10798))
* correctly calculate retry attempt ([#33](#33)) ([4c852e2](4c852e2))
* use extend instead of object.assign ([#37](#37)) ([8c8dcdd](8c8dcdd))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant