Retry deploy from URL in case of errors #188
Conversation
clients/mtaclient/mta_rest_client.go
Outdated
| return AsyncUploadJobResult{ | ||
| ClientActions: []string{"RETRY_UPLOAD"}, | ||
| }, fmt.Errorf("could not get async file upload job %s: %s, body: %s", jobId, resp.Status, string(bodyBytes)) | ||
| } | ||
| return AsyncUploadJobResult{}, fmt.Errorf("could not get async file upload job %s: %s, body: %s", jobId, resp.Status, string(bodyBytes)) |
There was a problem hiding this comment.
Reuse error message as variable
| @@ -153,7 +153,7 @@ func (c RetryableMtaRestClient) GetAsyncUploadJob(jobId string, namespace *strin | |||
| } | |||
| resp, err := baseclient.CallWithRetry(getAsyncUploadJobCb, c.MaxRetriesCount, c.RetryInterval) | |||
| if err != nil { | |||
| return AsyncUploadJobResult{}, err | |||
| return resp.(AsyncUploadJobResult), err | |||
There was a problem hiding this comment.
Why resp object must be returned now?
There was a problem hiding this comment.
So we can re-use the retry logic:
if resp.StatusCode == 400 { return AsyncUploadJobResult{ ClientActions: []string{"RETRY_UPLOAD"}, }, fmt.Errorf("%s %s: %s, body: %s", couldNotGetAsyncJobError, jobId, resp.Status, string(bodyBytes)) }
In this code the retry is explicitly set and it will be wiped out if we just return an empty object in the retryable client
commands/deploy_command.go
Outdated
| for i := 0; i < 3; i++ { | ||
| progressBar := c.tryFetchMtarSize(url, disableProgressBar) | ||
| uploadFromUrlStatus = c.doUploadFromUrl(encodedFileUrl, mtaClient, namespace, progressBar) | ||
| if shouldRetryUpload(uploadFromUrlStatus) { | ||
| ui.Warn("Retrying failed upload from URL operation") | ||
| continue | ||
| } | ||
| return uploadFromUrlStatus | ||
| } |
There was a problem hiding this comment.
Can't we use some general retry mechanism? This custom retry logic is not configurable via --retries command flag.
There was a problem hiding this comment.
Refactored it a bit, so it could be reused. We cannot reuse the other retries, as one is bound to the rest client and the other retries the MTA operation.
Do you think this retry logic should be bound to the --retries, imo it should be used only for retrying the operation, not the upload.
It will be inconsistent to use --retries here, because in the regular upload, there are 3 retries hardcoded
commands/deploy_command.go
Outdated
| func shouldRetryUpload(uploadFromUrlStatus UploadFromUrlStatus) bool { | ||
| if uploadFromUrlStatus.ExecutionStatus == Success { | ||
| return false | ||
| } | ||
| for _, clientAction := range uploadFromUrlStatus.ClientActions { | ||
| if clientAction == "RETRY_UPLOAD" { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we have similar retry mechanism when upload file to the backend of deploy-service?
There was a problem hiding this comment.
No, there's a single request/response, which will be retried by the rest client retry mechanism
IvanBorislavovDimitrov
force-pushed
the
response-code
branch
7 times, most recently
from
6ca6993
to
b6dfe07
Compare
commands/retrier/retry_util.go
Outdated
| time.Sleep(3 * time.Second) | ||
| continue | ||
| } | ||
| return result, nil |
There was a problem hiding this comment.
Should we return err variable instead nil? This way error won't be consumed in this function and will be propagated in the caller function.
There was a problem hiding this comment.
oops, fixed
| "github.com/cloudfoundry-incubator/multiapps-cli-plugin/log" | ||
| ) | ||
|
|
||
| func Execute[T any](attempts int, callback func() (T, error), shouldRetry func(result T, err error) bool) (T, error) { |
IvanBorislavovDimitrov
force-pushed
the
response-code
branch
from
b6dfe07
to
e1d7c7a
Compare
IvanBorislavovDimitrov
force-pushed
the
response-code
branch
from
e1d7c7a
to
fdee666
Compare
|
|
||
| func logError[T any](result T, err error) { | ||
| if err != nil { | ||
| log.Tracef("retrying an operation that failed with: %v", err) |
There was a problem hiding this comment.
intentionally lowercase
No description provided.