-
Notifications
You must be signed in to change notification settings - Fork 307
add retry for http retryable requests #1390
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
Conversation
|
🍰 |
|
Honestly this is a very bad change. There are no way to configure maximum retry attempts and no way to configure timeout. I had production issues because of this. |
|
now that we have the functionality and most of them are constants which can be extracted into client arguments for customisation. it needs some work yes. feel free to send a PR. |
| transport: Transport, | ||
| opt: https.RequestOptions, | ||
| body: Buffer | string | stream.Readable | null = null, | ||
| maxRetries: number = MAX_RETRIES, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is never respected and never called.
| } | ||
|
|
||
| const MAX_RETRIES = 10 | ||
| const EXP_BACK_OFF_BASE_DELAY = 1000 // Base delay for exponential backoff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base delay of 1 second is not a good value. Based on getExpBackoffDelay function, this will have a maximum value around 1009556 milliseconds, which is 16 minutes.
@prakashsvmx I will send a PR in a few hours. But what about the release? Can you put out a release shortly after that? |
add retry for http retryable requests
e.g:
# Try to make few drives offline or permission denied. to simulate it chmod 000 ./data{1..3}/ or revert back the permission like: chmod 755 -R ./data{1..3}/Sample runs.