commercetools / nodejs

Everything related to the Node.js ecosystem for the commercetools platform.
https://commercetools.github.io/nodejs/
MIT License
75 stars 70 forks source link

Add retryCodes option in HttpMiddlewareOptions #1764

Closed ajimae closed 2 years ago

ajimae commented 2 years ago

Summary

Description

This adds options to allow customers retry requests on a given status (error) codes.

Related Issue:

https://github.com/commercetools/nodejs/issues/1743

changeset-bot[bot] commented 2 years ago

⚠️ No Changeset found

Latest commit: 17b36c52dd2aa05600e57c5d4bb2ad41c164464f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codecov[bot] commented 2 years ago

Codecov Report

Merging #1764 (17b36c5) into master (ab6986b) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1764   +/-   ##
=======================================
  Coverage   94.28%   94.29%           
=======================================
  Files         137      137           
  Lines        4811     4817    +6     
  Branches     1275     1280    +5     
=======================================
+ Hits         4536     4542    +6     
  Misses        271      271           
  Partials        4        4           
Impacted Files Coverage Δ
packages/sdk-middleware-http/src/http.js 96.24% <100.00%> (+0.17%) :arrow_up:

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 ab6986b...17b36c5. Read the comment docs.

lojzatran commented 2 years ago

Hi @ajimae I don't think this retry is enough. As I wrote in the issue (https://github.com/commercetools/nodejs/issues/1743#issuecomment-1028794106), it should be able to retry also at network errors OR at 4xx errors if it contains a particular error message. Also we should be able to define the retry rules. Basically it should work the same as in the old sphere-node-sdk (https://github.com/commercetools/sphere-node-sdk/blob/master/src/coffee/repeater-task-queue.coffee#L7).

ajimae commented 2 years ago

Hi @ajimae I don't think this retry is enough. As I wrote in the issue (#1743 (comment)), it should be able to retry also at network errors OR at 4xx errors if it contains a particular error message. Also we should be able to define the retry rules. Basically it should work the same as in the old sphere-node-sdk (https://github.com/commercetools/sphere-node-sdk/blob/master/src/coffee/repeater-task-queue.coffee#L7).

Hi Lam,

Thanks for the feedback, I will work to include retries for the network errors and also specific error messages. For the retry rules, we already have this defined and configurable, except you have a rules that is not part of what we currently have, then please you can as well let us know.

Thanks

lojzatran commented 2 years ago

Hi @ajimae do you have an estimation when this PR will be merged and released? Thank you.

ajimae commented 2 years ago

Hi @ajimae do you have an estimation when this PR will be merged and released? Thank you.

I really can't say because we need @jenschude to approve.