-
Notifications
You must be signed in to change notification settings - Fork 19
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
Added retry logic #41
Added retry logic #41
Conversation
hassansin
commented
Jul 31, 2018
- Retry on 429, [500,600) statuses
- Retry on network releated errors
- Retry for maximum 15 mins
@@ -9,15 +9,15 @@ composer: | |||
@$(RUNNER) "composer $(filter-out $@,$(MAKECMDGOALS))" | |||
dependencies: | |||
make -s composer update -- --prefer-dist | |||
test: dependencies | |||
test: |
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.
Why not dependent on updating dependencies anymore?
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.
Installing dependencies before each command is good practice but not required. But unlike go get
, composer update
is very slow and I've to wait like 5-10 seconds before seeing any test output. Go frustrated and removed it.
|
||
protected function shouldRetry($attempt, $maxAttempts, ResponseInterface $response = null, \Exception $ex = null) | ||
{ | ||
if ($attempt >= $maxAttempts && ! is_null($ex)) { |
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.
Just some formatting here: ! is not near is_null
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.
hmm, wonder why php_codesniffer
didn't fix it 🤔
|
||
protected function shouldRetry($attempt, $maxAttempts, ResponseInterface $response = null, \Exception $ex = null) | ||
{ | ||
if ($attempt >= $maxAttempts && ! is_null($ex)) { |
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.
What happens if this is true $attempt >= $maxAttempts
, meaning it reached the maximum number of retries, but this is false !is_null($ex)
, meaning there was no exception thrown? It doesn't look like it will stop.
Shouldn't this check be with ||
instead of &&
?
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.
I would actually like to know the answer to @bogdanguranda question
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.
So if there is no exception and the HTTP status isn't 429/5xx, then the function would return null (falsy value) and we would never retry. So $attempt
would never exceed $maxAttempts
.
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.
Ok. And it will actually get here:
if (!is_null($response)) {
return $attempt < $maxAttempts && $this->retryHTTPStatus($response->getStatusCode());
}
And this will return false
which is correct, as attempts exceeded max attempts.
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.
LGTM
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.
🏅