Skip to content

Support client retries #227

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

Merged
merged 9 commits into from
Oct 21, 2021
Merged

Support client retries #227

merged 9 commits into from
Oct 21, 2021

Conversation

moritzraho
Copy link
Contributor

@moritzraho moritzraho commented Oct 7, 2021

This PR adds support for retries in the client using the 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.
Default values might be set differently too: https://github.com/apache/openwhisk-client-js/pull/227/files#diff-ca407d959d33ce92a7f7efee354a5ea5e6e61fcd797d32c04205e429a074ed54R160

This has been tested via unit tests and manually.

Api doc and README have been updated too.

@moritzraho
Copy link
Contributor Author

anyone wants to look at this ?
@rabbah maybe ?

Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

lgtm - minor nits.

@moritzraho
Copy link
Contributor Author

moritzraho commented Oct 14, 2021

Thanks for your reviews @selfxp and @rabbah !
Closed the comments, in for another quick review ?

@rabbah
Copy link
Member

rabbah commented Oct 16, 2021

@moritzraho is there a missing commit with the recent changes?

Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the added tests 🎉

@moritzraho
Copy link
Contributor Author

@moritzraho is there a missing commit with the recent changes?

Yeah haha 😅

@moritzraho
Copy link
Contributor Author

thanks for the review @rabbah

@moritzraho
Copy link
Contributor Author

Can we merge this one ? cc @rabbah @selfxp

@selfxp
Copy link
Contributor

selfxp commented Oct 18, 2021

It looks like there were some changes on how travis reports build statuses: travis-ci/travis-ci#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

@dgrove-oss
Copy link
Member

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).

@moritzraho
Copy link
Contributor Author

Maybe it's an issue in travis, anyone has access to verify ?

@dgrove-oss
Copy link
Member

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.

@dgrove-oss dgrove-oss closed this Oct 19, 2021
@dgrove-oss dgrove-oss reopened this Oct 19, 2021
@dgrove-oss
Copy link
Member

dgrove-oss commented Oct 19, 2021

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.

> [email protected] 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! [email protected] check-deps-size: `./tools/check_size.sh 1100`
2413npm ERR! Exit status 1
2414npm ERR! 
2415npm ERR! Failed at the [email protected] check-deps-size script.

@moritzraho
Copy link
Contributor Author

@dgrove-oss is travis triggered manually ?

@dgrove-oss dgrove-oss closed this Oct 20, 2021
@dgrove-oss dgrove-oss reopened this Oct 20, 2021
@dgrove-oss
Copy link
Member

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.

@selfxp selfxp merged commit 4dfc896 into apache:master Oct 21, 2021
@purplecabbage
Copy link
Contributor

Do we have an idea of when this would be released? @rabbah @dgrove-oss @selfxp

@selfxp
Copy link
Contributor

selfxp commented Nov 2, 2021

I'm not on the PMC, @tysonnorris will help release this tomorrow morning

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.

5 participants