Skip to content
This repository was archived by the owner on Mar 9, 2026. It is now read-only.

Allow shortcutting in customBackoff based on error#2898

Merged
AllanZhengYP merged 3 commits intoaws:masterfrom
nullren:shortcut-customBackoff
Nov 4, 2019
Merged

Allow shortcutting in customBackoff based on error#2898
AllanZhengYP merged 3 commits intoaws:masterfrom
nullren:shortcut-customBackoff

Conversation

@nullren
Copy link
Copy Markdown
Contributor

@nullren nullren commented Oct 10, 2019 •
edited
Loading

Adds the error as a parameter to the customBackoff function. This should be a non-breaking change for all clients.

If the result of the custom backoff is negative, then use that as a signal to shortcut remaining retries. This would make explicit a previously undefined behavior if clients returned a negative value from customBackoff.

Previously stated in issue #2896 (cc @ajredniwja):

Is your feature request related to a problem? Please describe.
It is frustrating when I add a custom backoff function and must wait for all retry attempts for errors I know will repeat if retried. Specifically, if I receive a NetworkingError: Invalid character in header content ["x-amz-copy-source"] error, I know this will be the same error for each request, there's no need for me to wait for all backoff attempts.

Describe the solution you'd like
Pass the error through to the customBackoff function and allow some return value to signal shortcutting the retry attempts, eg, returning negative value or throwing an Error. Example logic change, happy to open PR with remaining repo changes.

Describe alternatives you've considered
Not using built-in retry logic and handling it all ourselves.

Checklist
  • npm run test passes
  • .d.ts file is updated
  • changelog is added, npm run add-change
  • run bundle exec rake docs:api and inspect doc/latest/index.html if documentation is changed

@nullren nullren force-pushed the shortcut-customBackoff branch from 73bc534 to 2d3441b Compare October 10, 2019 22:19
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 10, 2019 •
edited
Loading

Codecov Report

Merging #2898 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2898   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files         300      300           
  Lines        8992     8992           
  Branches     1677     1677           
=======================================
  Hits         8719     8719           
  Misses        273      273
Impacted Files Coverage Δ
lib/config.js 87.64% <ø> (ø) ⬆️
lib/event_listeners.js 96.16% <100%> (ø) ⬆️
lib/util.js 94.73% <100%> (ø) ⬆️
lib/services/dynamodb.js 100% <100%> (ø) ⬆️
lib/service.js 98.14% <100%> (ø) ⬆️

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 fd804e1...22f780f. Read the comment docs.

@nullren nullren force-pushed the shortcut-customBackoff branch from 2d3441b to 8a00490 Compare October 10, 2019 22:27
AllanZhengYP
AllanZhengYP reviewed Oct 11, 2019
View reviewed changes
Comment thread lib/util.js
@AllanZhengYP
Copy link
Copy Markdown
Contributor

AllanZhengYP commented Oct 11, 2019

Thanks @nullren This is a long-losting feature we'd love to have

@nullren nullren requested a review from AllanZhengYP October 11, 2019 22:26
@AllanZhengYP
Copy link
Copy Markdown
Contributor

AllanZhengYP commented Oct 15, 2019

@nullren You don't need to change any source code in dist/ folder. They will be generated at every release. Can you remove those diffs?

Allow shortcutting in customBackoff based on error
5868bb3 
Adds the error as a parameter to the `customBackoff` function. This should be a non-breaking change for all clients.

If the result of the custom backoff is negative, then use that as a signal to shortcut remaining retries. This would make explicit a previously undefined behavior if clients returned a negative value from `customBackoff`.
@nullren nullren force-pushed the shortcut-customBackoff branch from 57e3753 to 5868bb3 Compare October 15, 2019 23:50
@nullren
Copy link
Copy Markdown
Contributor Author

nullren commented Oct 15, 2019

@nullren You don't need to change any source code in dist/ folder. They will be generated at every release. Can you remove those diffs?

@AllanFly120 just squashed everything into the same commit without those

@nullren
Copy link
Copy Markdown
Contributor Author

nullren commented Oct 24, 2019 •
edited
Loading

@AllanFly120 i fixed the TS parameter addressing the comment i missed. are there now any other next steps that i can do here to help drive this forward? 😅

@nullren
Copy link
Copy Markdown
Contributor Author

nullren commented Oct 28, 2019

@AllanFly120 sorry! just pinging again to see what i can do if i've missed anything else here.

@ajredniwja ajredniwja added investigating Issue has been looked at and needs deep dive work by OSDS. and removed needs-response labels Oct 28, 2019
@AllanZhengYP
Copy link
Copy Markdown
Contributor

AllanZhengYP commented Oct 28, 2019

@nullren Nice! The code change looks good to me now. To make sure the feature works as expected, can you add a test case spying on setTimeout to make sure the backoff timeout does change according the the customBackoff function and the error?

@nullren
Copy link
Copy Markdown
Contributor Author

nullren commented Nov 4, 2019

@AllanFly120 i hope i didn't misunderstand you. i added a new test in util.spec.js that added a spy to setTimeout and validated that the results from customBackoff are passed through.

AllanZhengYP
AllanZhengYP approved these changes Nov 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

ship it! 🚢

@AllanZhengYP AllanZhengYP merged commit 1ca7b98 into aws:master Nov 4, 2019
@nullren nullren deleted the shortcut-customBackoff branch November 4, 2019 23:58
@lock
Copy link
Copy Markdown

lock Bot commented Nov 12, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock Bot locked as resolved and limited conversation to collaborators Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@AllanZhengYP AllanZhengYP AllanZhengYP approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AllanZhengYP AllanZhengYP

Labels

investigating Issue has been looked at and needs deep dive work by OSDS.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nullren @codecov-io @AllanZhengYP @ajredniwja