badvision commented 3 years ago

Description

Curently, there is no retry logic in case of 5xx or 429 error responses. This adds retry with exponential backoff (1 sec, 2 sec, 4 sec). The final retry results in the same standard throwing behavior as before ala @adobe/node-fetch-retry

Also fixed: If you try to build the project right after checking out, you will get failures because of version discrepancies, requiring you to use --force.

Motivation and Context

Adobe.IO will throttle back if there are too many requests and clients receive 429 responses. This spaces out retries to prevent throttling as well as recover from 429s a little better.

How Has This Been Tested?

Empty out the node_modules folder and attempt to run npm i before applying this patch. Then try after applying this patch.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)

Checklist:

[x] I have signed the Adobe Open Source CLA.
[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

codecov[bot] commented 3 years ago

Codecov Report

Merging #17 (acd9021) into master (744dd42) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #17   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          341       365   +24     
  Branches        61        68    +7     
=========================================
+ Hits           341       365   +24

Impacted Files	Coverage Δ
src/SDKErrors.js	`100.00% <100.00%> (ø)`
src/helpers.js	`100.00% <100.00%> (ø)`
src/index.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 744dd42...acd9021. Read the comment docs.

badvision commented 3 years ago

@kmikawa please review when you get a chance. It's a simple change.

jdelbick commented 3 years ago

also, since this adds an option to the api, you may want/need to update the read me

badvision commented 3 years ago

also, since this adds an option to the api, you may want/need to update the read me

Done now, thanks! https://github.com/badvision/aio-lib-photoshop-api/blob/master/README.md#PhotoshopAPIOptions

badvision commented 3 years ago

The jest configuration file is in a slightly different directory than where the Jest tooling normally expects it. This helps IDE jest plugins a little with working, but then again it's probably better to omit IDE configurations in general. Bob had already checked something in the .vscode folder so I figured it was par for the course. If preferred I can remove this file from the PR also.

On Wed, Oct 13, 2021 at 1:10 PM tmathern @.***> wrote:

@.**** commented on this pull request.

In .vscode/launch.json https://github.com/adobe/aio-lib-photoshop-api/pull/17#discussion_r728328975 :

@@ -0,0 +1,29 @@ +{

Do we really want this file checked in?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adobe/aio-lib-photoshop-api/pull/17#pullrequestreview-778933688, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHH5BVMBTNUSQCXFC77O7LUGXDSPANCNFSM5FSMY2TQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

badvision commented 3 years ago

Since the comment identifies the delay is 1 second and the value is 1000 I think the conversion is pretty obvious in this case. But yes if I were using different values (like 30 seconds, etc) then I'd definitely make it clearer.

On Wed, Oct 13, 2021 at 1:15 PM tmathern @.***> wrote:

@.**** commented on this pull request.

In src/helpers.js https://github.com/adobe/aio-lib-photoshop-api/pull/17#discussion_r728329770 :

@@ -13,6 +13,13 @@ governing permissions and limitations under the License.

const loggerNamespace = 'aio-lib-photoshop-api'

const logger = @.***/aio-lib-core-logging')(loggerNamespace, { level: process.env.LOG_LEVEL })

+const NodeFetchRetry = @.***/node-fetch-retry')

+

+// Wait 1 second for first retry, then 2, 4, etc

+const RETRY_INITIAL_DELAY = 1000

Maybe add a comment that the unit is milliseconds? Or _MS in the variable name?

In src/helpers.js https://github.com/adobe/aio-lib-photoshop-api/pull/17#discussion_r728329818 :

@@ -13,6 +13,13 @@ governing permissions and limitations under the License.

const loggerNamespace = 'aio-lib-photoshop-api'

const logger = @.***/aio-lib-core-logging')(loggerNamespace, { level: process.env.LOG_LEVEL })

+const NodeFetchRetry = @.***/node-fetch-retry')

+

+// Wait 1 second for first retry, then 2, 4, etc

+const RETRY_INITIAL_DELAY = 1000

+

+// Retry for up to 14 seconds

+const RETRY_MAX_DURATON = 14000

Maybe add a comment that the unit is milliseconds? Or _MS in the variable name?

In src/helpers.js https://github.com/adobe/aio-lib-photoshop-api/pull/17#discussion_r728330164 :

@@ -32,6 +39,57 @@ function reduceError (error = {}) {

return error

}

+/**

Determine if we should retry fetch due to Server errors (server busy or other application errors)

*

@param {*} response Fetch response object, should at least have a status property which is the HTTP status code received

@returns {boolean} true if we should retry or false if not

*/

+function shouldRetryFetch (response = {}) {

return (response.status >= 500) || response.status === 429

Nitpick, so it reads nicer: ⬇️ Suggested change

return (response.status >= 500) || response.status === 429

return (response.status >= 500) || (response.status === 429)

In src/helpers.js https://github.com/adobe/aio-lib-photoshop-api/pull/17#discussion_r728330977 :

...retryOptions

}

const fetchFunction = (url, opts) => NodeFetchRetry(url, { ...options, ...opts })

// This is helpful for unit testing purposes

fetchFunction.isNodeFetchRetry = true

return fetchFunction

+}

+

+/**

Parse through options object and determine correct parameters to Swagger for desired fetch approach

*

@param {*} options Photoshop API options object

@returns {*} Swagger options object with relevant settings for fetch module

*/

+function getFetchOptions (options) {

if (options !== undefined && options.useSwaggerFetch) { // << TEST

I would also verify options isn't null (or use !!options).

In src/helpers.js https://github.com/adobe/aio-lib-photoshop-api/pull/17#discussion_r728331040 :

// This is helpful for unit testing purposes

fetchFunction.isNodeFetchRetry = true

return fetchFunction

+}

+

+/**

Parse through options object and determine correct parameters to Swagger for desired fetch approach

*

@param {*} options Photoshop API options object

@returns {*} Swagger options object with relevant settings for fetch module

*/

+function getFetchOptions (options) {

if (options !== undefined && options.useSwaggerFetch) { // << TEST

logger.debug('Using swagger fetch')

return {}

} else if (options !== undefined && options.userFetch !== undefined) { // << TEST

I would also verify options isn't null (or use !!options).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adobe/aio-lib-photoshop-api/pull/17#pullrequestreview-778934688, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHH5BX4GBOAWFN7HZ72Y43UGXEDJANCNFSM5FSMY2TQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

badvision commented 3 years ago

@kmikawa, @bobvanmanen -- I am unable to merge this PR, can one of you folks see if you have the ability? Thanks! :)

kmikawa commented 3 years ago

Hi @badvision

1. I think you need to run `npm run generate-docs` before merge to update readme. (@bobvanmanen, please correct me if I’m wrong.)

2. I just run unit test, and the coverage is decreased. Could you please check and add unit tests to meet 100% coverage?

badvision commented 3 years ago

Hey there @kmikawa ! Thanks for the quick reply. :)

1: Done. PR is updated with that

2: This is a strange one. The coverage shows 100% of the lines are covered but 49 of 50 statements are covered in the helper.js file. The line itself is executed over 60 times in the testing. What this is doing is concatenating objects and it is clear by the testing that this actually works in reality. So I'm not really sure how to get better coverage for this bit (line 68 which is: `const fetchFunction = (url, opts) => NodeFetchRetry(url, { ...options, ...opts })`

Is it acceptable to press forward with this PR or can you think of a way to cover this lambda statement?

kmikawa commented 3 years ago

Hey there @kmikawa ! Thanks for the quick reply. :)

1: Done. PR is updated with that #2: This is a strange one. The coverage shows 100% of the lines are covered but 49 of 50 statements are covered in the helper.js file. The line itself is executed over 60 times in the testing. What this is doing is concatenating objects and it is clear by the testing that this actually works in reality. So I'm not really sure how to get better coverage for this bit (line 68 which is: const fetchFunction = (url, opts) => NodeFetchRetry(url, { ...options, ...opts })

Is it acceptable to press forward with this PR or can you think of a way to cover this lambda statement?

Thank you for the update, @badvision!
@khound, can BYS help on this?

adobe-bot commented 1 year ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

adobe / aio-lib-photoshop-api

Feature: Use node-fetch-retry and add retry attempts for 429 responses #17

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Codecov Report

@.**** commented on this pull request.

@.**** commented on this pull request.

Maybe add a comment that the unit is milliseconds? Or _MS in the variable name?

Maybe add a comment that the unit is milliseconds? Or _MS in the variable name?

I would also verify options isn't null (or use !!options).

1. I think you need to run `npm run generate-docs` before merge to update readme. (@bobvanmanen, please correct me if I’m wrong.)

2. I just run unit test, and the coverage is decreased. Could you please check and add unit tests to meet 100% coverage?

1: Done. PR is updated with that

adobe / aio-lib-photoshop-api

Feature: Use node-fetch-retry and add retry attempts for 429 responses #17

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Codecov Report

@.**** commented on this pull request.

@.**** commented on this pull request.

Maybe add a comment that the unit is milliseconds? Or _MS in the variable name?

Maybe add a comment that the unit is milliseconds? Or _MS in the variable name?

I would also verify options isn't null (or use !!options).

1. I think you need to run npm run generate-docs before merge to update readme. (@bobvanmanen, please correct me if I’m wrong.)

2. I just run unit test, and the coverage is decreased. Could you please check and add unit tests to meet 100% coverage?

1: Done. PR is updated with that

1. I think you need to run `npm run generate-docs` before merge to update readme. (@bobvanmanen, please correct me if I’m wrong.)