application-research / autoretrieve

A server to make GraphSync data accessible on IPFS
22 stars 7 forks source link

feat: skip query phase for SPs that are already at max concurrent retrievals #119

Closed rvagg closed 2 years ago

rvagg commented 2 years ago

We're seeing a lot of entries in the event recorder DB for successful queries that don't go to retrievals (for any SP), and at the same time for some SPs we're seeing excessive numbers of queries. I believe this is simply because we're already at max concurrent retrievals for that SP and we're performing queries but not getting to do a retrieval for them because we only check concurrent count once we get to retrieval phase.

So this change will short-circuit an SP's inclusion in the candidate list if we're already at max concurrent retrievals for that SP. It's not always going to be the case that if we're at max-concurrent that we won't be able to retrieve, there's a small window where a retrieval could finish before we finish query phase; but it's more likely that we'll just end up bothering SPs with excessive pointless queries that don't result in a retrieval.

codecov-commenter commented 2 years ago

Codecov Report

Base: 14.04% // Head: 13.98% // Decreases project coverage by -0.06% :warning:

Coverage data is based on head (a038aaf) compared to base (6a312a1). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #119 +/- ## ========================================== - Coverage 14.04% 13.98% -0.07% ========================================== Files 16 16 Lines 2207 2217 +10 ========================================== Hits 310 310 - Misses 1882 1892 +10 Partials 15 15 ``` | [Impacted Files](https://codecov.io/gh/application-research/autoretrieve/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=application-research) | Coverage Δ | | |---|---|---| | [filecoin/activeretrievals.go](https://codecov.io/gh/application-research/autoretrieve/pull/119/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=application-research#diff-ZmlsZWNvaW4vYWN0aXZlcmV0cmlldmFscy5nbw==) | `66.66% <0.00%> (-3.18%)` | :arrow_down: | | [filecoin/retriever.go](https://codecov.io/gh/application-research/autoretrieve/pull/119/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=application-research#diff-ZmlsZWNvaW4vcmV0cmlldmVyLmdv) | `0.00% <0.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=application-research). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=application-research)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

rvagg commented 2 years ago

I'll wait for @hannahhoward for this one because I don't know how to deploy this to the bedrock kubernetes - I'd like to know though.