Current timeout is 30s, this add a second kind of timeout : if connection to remote can’t be established in 5s, the remote is considered down, so that a thread don’t stall for too long for no reason (and also make some unit test much faster to run)

elegaanz (Migrated from github.com) approved these changes 1 year ago

It makes me think: couldn’t we have a unique client (in a lazy_static! or something like that). I think reqwest keep connections for a certain time if the client is not dropped, and it probably caches DNS too.

Otherwise, it looks good to me, so if the CI is happy you can merge.

Codecov Report

Merging #557 into master will increase coverage by 2.71%.
The diff coverage is 62.96%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   40.03%   42.75%   +2.71%     
==========================================
  Files          69       70       +1     
  Lines        8035     8846     +811     
==========================================
+ Hits         3217     3782     +565     
- Misses       4818     5064     +246

maybe not a unique client (if we have multiple request to do at the same time they will be queued), but we could probably make some kind of pool of clients. I’ll keep the idea somewhere for when I have spare time to spend

👍 1

Reviewers

Plume_migration_agent approved these changes 1 year ago

The pull request has been merged as 33a0c7dcd3.

Plume_migration_agent

A: Backend

A: Federation

S: Ready for review

No Milestone

No Assignees

2 Participants

Notifications

Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

#557 reduce reqwest timeout to 5s

Codecov Report

Reviewers