feat: store legacy claims in cache #57
Conversation
Codecov ReportAttention: Patch coverage is
|
| legacyClaims := cfg.legacyClaims | ||
| if legacyClaims == nil { | ||
| var legacyClaims providerindex.LegacyClaimsFinder | ||
| if cfg.legacyClaimsMapper != nil && cfg.legacyClaimsBucket != nil { |
There was a problem hiding this comment.
Also check the URL is non-nil?
There was a problem hiding this comment.
the URL is less critical as the service can be built without it ("" might even be a valid value?) so I didn't bother to add it to the check. I can add it if you think it's important though.
There was a problem hiding this comment.
I think it needs a URL with "{claim}" in it otherwise the query processor will error...
There was a problem hiding this comment.
I added an explicit check to fail fast if the URL doesn't contain the placeholder. I felt that silently returning a NotFoundLegacyClaimsFinder if cfg.legacyClaimsUrl == "" could lead to errors difficult to find.
pkg/service/providerindex/legacy.go
Outdated
| @@ -89,6 +98,10 @@ func (ls LegacyClaimsStore) Find(ctx context.Context, contentHash multihash.Mult | |||
| } | |||
|
|
|||
| results = append(results, pr) | |||
|
|
|||
| if err := ls.claimsCache.Set(ctx, claimCid, claim, true); err != nil { | |||
There was a problem hiding this comment.
Won't it stay in the cache forever if we always store after retrieve?
There was a problem hiding this comment.
since the claims service will later fetch from the cache, I thought refreshing its expiration would be a good idea. IIUC the entry will be there only as long as it is being hit, but will be removed if nobody asks for it in the expiration time. Does that sound correct?
There was a problem hiding this comment.
That sounds reasonable...but the point of caching it is to ensure it's super fast on subsequent reads. Here we'd be enforcing a write on read for every request, at some number of requests it's going to be slower on average than letting it fall out of the cache and revalidate. Also if it is removed from the underlying store, and keeps getting refreshed then it will never expire.
There was a problem hiding this comment.
thanks! That reasoning makes total sense. I updated the code so that claims are not written to the cache if they were fetched from there.
|
@alanshaw @volmedo while caching from the lookup is a good optimization, I do not believe it's a full strategy. You've got a potential fail path here if:
Generally, I think the better approach here is to generate wrappable interfaces. I will work on a refactor suggestion. |
|
suggested refactor #80 |
resolves #55