[Ref] Use DNS text queries to resolve PCI IDs #3528
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CommandMC
requested review from
arielj,
flavioislima,
Etaash-mathamsetty,
Nocccer and
imLinguin
and removed request for
a team
|
I'm not sure I understand the reason to clear the cache after 7 days. If the information that's cached already matches my GPU, why would we need to discard it and do a new request? it's because the name of my device can change in that endpoint? |
|
Correct, device names can change (they don't do often, but they might) 7 days might indeed still be too frequent for most use cases, but my layman's guess would be that one of these queries per user every 7 days still is not much at all in terms of data transferred |
|
I am pretty fine with a couple of requests per 7 days per user. |
We only want to clear them if we're *not* upgrading Heroic though (don't want to cause a request spike when releasing an update)
CommandMC
force-pushed
the
ref/pci-ids-dns
branch
from
dde2c0c to
5434bcc
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #3483
To test this:
I've tested this on both real and virtual machines, and on both Windows and Linux. macOS does not need testing, as detecting GPUs in the first place isn't done there.
Internally, we now keep two
CacheStores for (1) vendor names and (2) device names. The vendor name cache is simply indexed by their ID, device names are indexed with either<DeviceSubID>_<VendorSubId>_<DeviceId>_<VendorId>, if there are sub-IDs, or just<DeviceId>_<VendorId>if there aren't. This key format is handy, since replacing the_with.nets us the DNS query prefix.Both caches are valid for 7 days. I've toned this down from 30, but the bandwidth used should still be much less than before (since we're just requesting individual IDs).
CC @gollux
This requires #3527, as
downloadFileisn't used anymore (so Unimported will fail)Use the following Checklist if you have changed something on the Backend or Frontend: