-
Notifications
You must be signed in to change notification settings - Fork 2k
Adds backoff / retry to HTTP calls. #545
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #545 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 3 3
=====================================
Hits 3 3 Continue to review full report at Codecov.
|
iot/http_example/package.json
Outdated
"jsonwebtoken": "7.4.1", | ||
"request": "2.82.0", | ||
"uuid": "3.1.0" | ||
"requestretry": "requestretry", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a SemVer number? e.g. 1.12.2
, the latest version of the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched.
@@ -182,6 +201,21 @@ function publishAsync (authToken, messageCount, numMessages) { | |||
// [START iot_http_getconfig] | |||
function getConfig (authToken, version) { | |||
console.log(`Getting config from URL: ${urlBase}`); | |||
|
|||
// Custom backoff functions | |||
function retryNotSuccess (err, response, body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a library that comes with these built-in? (Perhaps nodejs-repo-tools?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, switched to recommended library that uses built-in exponential backoff.
@@ -141,16 +141,35 @@ function publishAsync (authToken, messageCount, numMessages) { | |||
binary_data: binaryData | |||
} | |||
}; | |||
|
|||
// Custom backoff functions | |||
function retryNotSuccess (err, response, body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto to below comment - can these come from a library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, switched to recommended library that uses built-in exponential backoff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once nits are fixed. 👍
}; | ||
|
||
// Send events for high-frequency updates, update state only occasionally. | ||
const delayMs = argv.messageType === 'events' ? 1000 : 2000; | ||
request.post(options, function (error, response, body) { | ||
console.log(JSON.stringify(request)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended?
@@ -17,7 +17,7 @@ | |||
// [START iot_http_includes] | |||
const fs = require('fs'); | |||
const jwt = require('jsonwebtoken'); | |||
const request = require('requestretry'); | |||
const request = require('retry-request'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we name this something other than request
(to avoid confusion with the library of the same name)? Maybe retryRequest?
function (incomingHttpMessage) { | ||
console.log('Retry?'); | ||
return incomingHttpMessage.statusMessage !== 'OK'; | ||
} | ||
}; | ||
console.log(JSON.stringify(request.RetryStrategies)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto - is this intended?
}; | ||
console.log(JSON.stringify(request.RetryStrategies)); | ||
request.get(options, function (error, response, body) { | ||
request(options, function (error, response, body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: arrow function?
(Apply this comment throughout your PR if you decide to make any changes.)
🤖 I have created a release \*beep\* \*boop\* --- ### [2.3.4](https://www.github.com/googleapis/nodejs-tasks/compare/v2.3.3...v2.3.4) (2021-06-30) ### Bug Fixes * **deps:** google-gax v2.17.0 with mTLS ([#543](https://www.github.com/googleapis/nodejs-tasks/issues/543)) ([d1a715d](https://www.github.com/googleapis/nodejs-tasks/commit/d1a715d92e0136b1840798c2e122e165201715cd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release \*beep\* \*boop\* --- ### [2.3.4](https://www.github.com/googleapis/nodejs-tasks/compare/v2.3.3...v2.3.4) (2021-06-30) ### Bug Fixes * **deps:** google-gax v2.17.0 with mTLS ([#543](https://www.github.com/googleapis/nodejs-tasks/issues/543)) ([d1a715d](https://www.github.com/googleapis/nodejs-tasks/commit/d1a715d92e0136b1840798c2e122e165201715cd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
No description provided.