You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2021/10/07 15:24:44 UTC
[GitHub] [openwhisk-client-js] moritzraho opened a new pull request #227: Support client retries
moritzraho opened a new pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227
This PR adds support for retries in the client using the [async-retry](https://www.npmjs.com/package/async-retry) package. Like now, the default is no retries.
Retries are performed on http errors and all 4xx and 5xx error codes. This can be re-discussed, e.g. retries on 4xx might not be wanted.
This has been tested via unit tests and manually.
Api doc and README have been updated too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-946076327
It looks like there were some changes on how travis reports build statuses: https://github.com/travis-ci/travis-ci/issues/10204.
@rabbah @dgrove-oss would you be able to make these git project changes as follows (i don't have access to project's settings):
```
- Go to the repository’s Branch Settings page (https://github.com/apache/openwhisk-client-js/settings/branches).
- In the "Branch Protection Settings" section, click "Edit" for a protected branch
- Scroll down to the second box, "Require status checks to pass before merging." Select either "Travis CI - Pull Request" or "Travis CI - Branch" or both. (Note: "continuous-integration/travis-ci" is the Status API event which is deprecated)
- Save your changes
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] purplecabbage commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
purplecabbage commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-956541561
Do we have an idea of when this would be released? @rabbah @dgrove-oss @selfxp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] rabbah commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
rabbah commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-944919085
@moritzraho is there a missing commit with the recent changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] rabbah commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r728934325
##########
File path: test/unit/client.test.js
##########
@@ -22,18 +22,60 @@ const Client = require('../../lib/client')
const http = require('http')
const nock = require('nock')
-// Note: this has to come before any of the proxy tests, as they interfere
+// Note: All client.request tests have to come before any of the proxy tests, as they interfere
+
+test('should return response', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'all good')
+ nock.removeInterceptor(interceptor)
+})
+
test('should handle http request errors', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
- nock('https://test_host').get(PATH).replyWithError('simulated error')
+ const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
})
+test('should support retries on error', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(2).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'now all good')
Review comment:
To confirm my understanding of `times(2)`, the path will respond with the provided value `2 times` before the next mocked response?
(Alternatively, is there a way to confirm the retry actually was called with another test assertion (passing a function to `retry` for a callback) to confirm both the function was called as in this case and later that it is not called when no retry is specified.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r728395861
##########
File path: test/unit/client.test.js
##########
@@ -22,18 +22,60 @@ const Client = require('../../lib/client')
const http = require('http')
const nock = require('nock')
-// Note: this has to come before any of the proxy tests, as they interfere
+// Note: All client.request tests have to come before any of the proxy tests, as they interfere
+
+test('should return response', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'all good')
+ nock.removeInterceptor(interceptor)
+})
+
test('should handle http request errors', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
- nock('https://test_host').get(PATH).replyWithError('simulated error')
+ const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
})
+test('should support retries on error', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(2).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'now all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'now all good')
+ nock.removeInterceptor(interceptor)
+})
+
+test('should handle errors even after retries', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(3).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'not enough retries to come here')
+ const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
+ t.truthy(error.message)
+ t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
+})
+
Review comment:
Should we add a test to validate that indeed no retry is made in case no retry configuration exists? Something like:
```
test('should not retry when no retry config available', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
const interceptor = nock('https://test_host')
.get(PATH).times(1).replyWithError('simulated error')
.get(PATH).times(1).reply(200, 'now all good')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
nock.removeInterceptor(interceptor)
})
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho edited a comment on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho edited a comment on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-943418180
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] rabbah commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r728930963
##########
File path: README.md
##########
@@ -105,7 +105,7 @@ _Client constructor supports the following mandatory parameters:_
- **key**. Client key to use when connecting to the `apihost` (if `nginx_ssl_verify_client` is turned on in your apihost)
- **proxy.** HTTP(s) URI for proxy service to forwards requests through. Uses Needle's [built-in proxy support](https://github.com/tomas/needle#request-options).
- **agent.** Provide custom [http.Agent](https://nodejs.org/api/http.html#http_class_http_agent) implementation.
-
+- **retry**. Provide a retry options to retry on errors, for example, `{ retries: 2 }`. By default, no retries will be done. Uses [async-retry options](https://github.com/vercel/async-retry#api). Default values are different from async-retry, please refer to the api doc.
Review comment:
```suggestion
- **retry**. Provide a retry options to retry on errors, for example, `{ retries: 2 }`. By default, no retries will be done. Uses [async-retry options](https://github.com/vercel/async-retry#api). Default values are different from async-retry, please refer to the API doc.
```
##########
File path: test/unit/client.test.js
##########
@@ -22,18 +22,60 @@ const Client = require('../../lib/client')
const http = require('http')
const nock = require('nock')
-// Note: this has to come before any of the proxy tests, as they interfere
+// Note: All client.request tests have to come before any of the proxy tests, as they interfere
+
+test('should return response', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'all good')
+ nock.removeInterceptor(interceptor)
+})
+
test('should handle http request errors', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
- nock('https://test_host').get(PATH).replyWithError('simulated error')
+ const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
})
+test('should support retries on error', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(2).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'now all good')
Review comment:
Is there a way to confirm the retry actually was called with another test assertion (passing a function to `retry` for a callback)?
##########
File path: test/unit/client.test.js
##########
@@ -22,18 +22,60 @@ const Client = require('../../lib/client')
const http = require('http')
const nock = require('nock')
-// Note: this has to come before any of the proxy tests, as they interfere
+// Note: All client.request tests have to come before any of the proxy tests, as they interfere
+
+test('should return response', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'all good')
+ nock.removeInterceptor(interceptor)
+})
+
test('should handle http request errors', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
- nock('https://test_host').get(PATH).replyWithError('simulated error')
+ const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
})
+test('should support retries on error', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(2).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'now all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'now all good')
+ nock.removeInterceptor(interceptor)
+})
+
+test('should handle errors even after retries', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(3).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'not enough retries to come here')
+ const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
+ t.truthy(error.message)
+ t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
+})
+
Review comment:
+1 - as I was reading the tests I was thinking the same thing (confirm both presence and absence of the retry).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-945761975
Can we merge this one ? cc @rabbah @selfxp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-947582222
@dgrove-oss is travis triggered manually ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss closed pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss closed pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r729009003
##########
File path: lib/client.js
##########
@@ -135,7 +150,21 @@ class Client {
throw new Error(`${messages.INVALID_OPTIONS_ERROR} Missing either api or apihost parameters.`)
}
- return { apiKey: apiKey, api, apiVersion: apiversion, ignoreCerts: ignoreCerts, namespace: options.namespace, apigwToken: apigwToken, apigwSpaceGuid: apigwSpaceGuid, authHandler: options.auth_handler, noUserAgent: options.noUserAgent, cert: options.cert, key: options.key, proxy, agent }
+ // gather retry options
+ const retry = options.retry
+ if (retry && typeof options.retry !== 'object') {
+ throw new Error(`${messages.INVALID_OPTIONS_ERROR} 'retry' option must be an object, e.g. '{ retries: 2 }'.`)
+ }
+ if (retry) {
+ // overwrite async-retry defaults, see https://github.com/vercel/async-retry#api for more details
+ retry.retries = retry.retries || 2
+ retry.factor = retry.factor || 2
+ retry.minTimeout = retry.minTimeout || 100
Review comment:
@selfxp was thinking about the case where openwhisk-js is used from a web-action action, 1 second min timeout might be too long
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r729045274
##########
File path: test/unit/client.test.js
##########
@@ -22,18 +22,60 @@ const Client = require('../../lib/client')
const http = require('http')
const nock = require('nock')
-// Note: this has to come before any of the proxy tests, as they interfere
+// Note: All client.request tests have to come before any of the proxy tests, as they interfere
+
+test('should return response', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'all good')
+ nock.removeInterceptor(interceptor)
+})
+
test('should handle http request errors', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
- nock('https://test_host').get(PATH).replyWithError('simulated error')
+ const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
})
+test('should support retries on error', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(2).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'now all good')
Review comment:
done + allowed to add a test for no retry on success
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-943418180
Thanks for your reviews @selfxp and @rabbah !
Closed the comments, in for another quick round of review ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-947044037
Travis is triggering now, but there's a failure. Wild guessing, I would speculate it that the PR added some additional dependencies and we failed the size check.
```
> openwhisk@3.21.4 check-deps-size /home/travis/build/apache/openwhisk-client-js
2402> ./tools/check_size.sh 1100
2403
2404Checking node_modules size isn't more than threshold (kb): 1100
2405openwhisk-3.21.4.tgz
2406added 8 packages from 8 contributors and audited 747 packages in 8.63s
2407found 13 vulnerabilities (12 moderate, 1 high)
2408 run `npm audit fix` to fix them, or `npm audit` for details
2409failure! node_modules size (1104) is more than threshold
2410npm ERR! code ELIFECYCLE
2411npm ERR! errno 1
2412npm ERR! openwhisk@3.21.4 check-deps-size: `./tools/check_size.sh 1100`
2413npm ERR! Exit status 1
2414npm ERR!
2415npm ERR! Failed at the openwhisk@3.21.4 check-deps-size script.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-946927874
I noticed in travis.yml we are requesting a node10 environment, which is long out of support. I'm trying to drop that in #228 and see if that helps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-940100017
anyone wants to look at this ?
@rabbah maybe ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] purplecabbage commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
purplecabbage commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-956541561
Do we have an idea of when this would be released? @rabbah @dgrove-oss @selfxp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-946182561
We can't change things through the GitHub UI, but can configure via .asf.yaml. I believe it is already set correctly (https://github.com/apache/openwhisk-client-js/blob/master/.asf.yaml#L30-L38).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-948109240
travis is clean. @rabbah or @selfxp if you are happy with changes go ahead and merge. We did need to bump up the arbitrary limit of 1000 dependencies, but since async-retry was added that is probably not unexpected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-944919895
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r728397971
##########
File path: lib/client.js
##########
@@ -135,7 +150,21 @@ class Client {
throw new Error(`${messages.INVALID_OPTIONS_ERROR} Missing either api or apihost parameters.`)
}
- return { apiKey: apiKey, api, apiVersion: apiversion, ignoreCerts: ignoreCerts, namespace: options.namespace, apigwToken: apigwToken, apigwSpaceGuid: apigwSpaceGuid, authHandler: options.auth_handler, noUserAgent: options.noUserAgent, cert: options.cert, key: options.key, proxy, agent }
+ // gather retry options
+ const retry = options.retry
+ if (retry && typeof options.retry !== 'object') {
+ throw new Error(`${messages.INVALID_OPTIONS_ERROR} 'retry' option must be an object, e.g. '{ retries: 2 }'.`)
+ }
+ if (retry) {
+ // overwrite async-retry defaults, see https://github.com/vercel/async-retry#api for more details
+ retry.retries = retry.retries || 2
+ retry.factor = retry.factor || 2
+ retry.minTimeout = retry.minTimeout || 100
Review comment:
I personally prefer the default `1000`, but given the fact that customers can reconfigure this value, `100` should be fine too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss closed pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss closed pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-946900997
Maybe it's an issue in travis, anyone has access to verify ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-957000985
I'm not on the [PMC](https://projects.apache.org/committee.html?openwhisk), @tysonnorris will help release this tomorrow morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-957000985
I'm not on the [PMC](https://projects.apache.org/committee.html?openwhisk), @tysonnorris will help release this tomorrow morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] moritzraho commented on a change in pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
moritzraho commented on a change in pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#discussion_r728981025
##########
File path: test/unit/client.test.js
##########
@@ -22,18 +22,60 @@ const Client = require('../../lib/client')
const http = require('http')
const nock = require('nock')
-// Note: this has to come before any of the proxy tests, as they interfere
+// Note: All client.request tests have to come before any of the proxy tests, as they interfere
+
+test('should return response', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host').get(PATH).times(1).reply(200, 'all good')
+ const result = await client.request(METHOD, PATH, {})
+ t.is(result.toString(), 'all good')
+ nock.removeInterceptor(interceptor)
+})
+
test('should handle http request errors', async t => {
const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '' })
const METHOD = 'GET'
const PATH = '/some/path'
- nock('https://test_host').get(PATH).replyWithError('simulated error')
+ const interceptor = nock('https://test_host').get(PATH).times(1).replyWithError('simulated error')
const error = await t.throwsAsync(client.request(METHOD, PATH, {}))
t.truthy(error.message)
t.assert(error.message.includes('simulated error'))
+ nock.removeInterceptor(interceptor)
})
+test('should support retries on error', async t => {
+ const client = new Client({ api_key: 'secret', apihost: 'test_host', proxy: '', retry: { retries: 2 } })
+ const METHOD = 'GET'
+ const PATH = '/some/path'
+
+ const interceptor = nock('https://test_host')
+ .get(PATH).times(2).replyWithError('simulated error')
+ .get(PATH).times(1).reply(200, 'now all good')
Review comment:
yes will give a try with the `onRetry` callback option
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp merged pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp merged pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] dgrove-oss edited a comment on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
dgrove-oss edited a comment on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-947044037
Travis is triggering now, but there's a failure. Wild guessing, I would speculate that the PR added some additional dependencies and we failed the size check.
```
> openwhisk@3.21.4 check-deps-size /home/travis/build/apache/openwhisk-client-js
2402> ./tools/check_size.sh 1100
2403
2404Checking node_modules size isn't more than threshold (kb): 1100
2405openwhisk-3.21.4.tgz
2406added 8 packages from 8 contributors and audited 747 packages in 8.63s
2407found 13 vulnerabilities (12 moderate, 1 high)
2408 run `npm audit fix` to fix them, or `npm audit` for details
2409failure! node_modules size (1104) is more than threshold
2410npm ERR! code ELIFECYCLE
2411npm ERR! errno 1
2412npm ERR! openwhisk@3.21.4 check-deps-size: `./tools/check_size.sh 1100`
2413npm ERR! Exit status 1
2414npm ERR!
2415npm ERR! Failed at the openwhisk@3.21.4 check-deps-size script.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] purplecabbage commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
purplecabbage commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-956541561
Do we have an idea of when this would be released? @rabbah @dgrove-oss @selfxp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-client-js] selfxp commented on pull request #227: Support client retries
Posted by GitBox <gi...@apache.org>.
selfxp commented on pull request #227:
URL: https://github.com/apache/openwhisk-client-js/pull/227#issuecomment-957000985
I'm not on the [PMC](https://projects.apache.org/committee.html?openwhisk), @tysonnorris will help release this tomorrow morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org