Conversation
bnewbold
left a comment
bnewbold
left a comment
There was a problem hiding this comment.
left some comments.
overall seems relatively straight-forward and low-risk to merge 👍
| NSIDFilter string | ||
| SyncRequestsPerSecond int | ||
| RelayHost string | ||
| // Per-PDS rate limit (requests per second per host). If 0, defaults to 2. |
There was a problem hiding this comment.
this logic is a bit weird to me, i'd expect the default to be 2, but 0 to mean no limit?
| } | ||
|
|
||
| // Wait on global rate limiter | ||
| b.syncLimiter.Wait(ctx) |
There was a problem hiding this comment.
should handle the possible err returned here (and two lines below). those would happen if the ctx is cancelled. otherwise the error will come from the client.Do(), and it would naively look like an HTTP request timeout.
| return instrumentedReader, nil | ||
| } | ||
|
|
||
| resp.Body.Close() |
There was a problem hiding this comment.
this is the HTTP non-success status code path. I think you need to drain the body here, or the connection can't be reused (eg, HTTP/2), which is relevant b/c you're probably making many requests to a PDS, and there will be some suspended/deleted accounts.
could also be nice to fetch error response JSON body and log it or something. doesn't need to happen in this PR but a TODO comment would help track it.
2 participants
A global rate limit is a little bad, so I added some retries on 429's, and also a per-PDS rate limiter