Skip to content
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

Retries for media fetch #138

Merged
merged 3 commits into from
Jul 7, 2023
Merged

Retries for media fetch #138

merged 3 commits into from
Jul 7, 2023

Conversation

vurpo
Copy link
Contributor

@vurpo vurpo commented Jul 3, 2023

I initially tried making use of the existing request with retry logic (the executeCompiledRequest method) but it didn't seem well suited for this kind of request and the code turned out ugly, so I made a copy of it to adapt it to this use better.

The alternative (reusing the existing method) would have looked like

	var resp *http.Response
	_, err := cli.MakeFullRequest(FullRequest{
		Method:  http.MethodGet,
		URL:     cli.GetDownloadURL(mxcURL),
		Context: ctx,
		Handler: func(req *http.Request, res *http.Response, _ interface{}) ([]byte, error) {
			resp = res
			return []byte{}, nil
		},
	})
	if err != nil {
		return nil, err
	}
	return resp, nil

instead, which is a much smaller change but I thought it looked ugly.

@vurpo vurpo requested review from tulir and sumnerevans July 3, 2023 14:17
Copy link
Member

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

I'd be interested to see what @tulir thinks about ideas to refactor this to reduce code duplication with the other retry path, but overall, it looks pretty reasonable.

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
Copy link
Member

@tulir tulir left a comment

Choose a reason for hiding this comment

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

Looks about right

(does it work?)

@vurpo
Copy link
Contributor Author

vurpo commented Jul 7, 2023

Works according to my testing now

@vurpo vurpo merged commit 4639f0c into master Jul 7, 2023
6 checks passed
@vurpo vurpo deleted the max/be-12122 branch July 7, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants