Page MenuHomePhabricator
Differential D13582

[lib] cache led channels response from neynar
ClosedPublic
Actions

Authored by varun on Wed, Oct 2, 10:05 PM.
Tags
None
Referenced Files
F2935329: D13582.id44848.diff
Thu, Oct 10, 7:22 AM
F2934184: D13582.diff
Thu, Oct 10, 5:17 AM
F2932072: D13582.id44848.diff
Wed, Oct 9, 11:20 PM
Unknown Object (File)
Tue, Oct 8, 8:15 PM
Unknown Object (File)
Mon, Oct 7, 7:48 PM
Unknown Object (File)
Thu, Oct 3, 5:16 AM
Unknown Object (File)
Thu, Oct 3, 4:42 AM
Unknown Object (File)
Thu, Oct 3, 4:42 AM
View All 13 Files
Subscribers
tomek

Details

Reviewers
ashoat
Commits
rCOMMf42a0f28297b: [lib] cache led channels response from neynar
Summary

we call this client method a lot from keyserver when a farcaster user auto joins communities. we should cache the response from neynar for a minute to avoid making unnecessary calls.

part of: https://linear.app/comm/issue/ENG-9473/app-freezes-when-user-with-a-popular-farcaster-user-joins

Test Plan

checked how many times the neynar API was called and it went down from hundreds to just 5

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Wed, Oct 2, 10:05 PM
Herald added a subscriber: tomek. · View Herald TranscriptWed, Oct 2, 10:05 PM
varun added a comment.Wed, Oct 2, 10:10 PM

maybe an LRU cache would've made more sense than a TTL

Harbormaster completed remote builds in B32031: Diff 44848.Wed, Oct 2, 10:22 PM
varun requested review of this revision.Wed, Oct 2, 10:22 PM
ashoat accepted this revision.Thu, Oct 3, 12:01 AM

We can go with this approach for now, but I think it would be better to replace the API so we're only querying one channel at a time. It's true that we'd end up with N Neynar queries instead of 1 Neynar query for N channels that get cached, but I think it's better to avoid the massive query at the start, and hopefully it's not expensive to do single "point queries" to Neynar

lib/utils/neynar-client.js
172

I don't think it makes sense to keep this API here. We should replace it with one that takes an FID and a channel, and we could cache the results of that one

176

Do we really need to re-check the TTL here after we just called this.cleanExpiredCacheEntries? I get that the current timestamp might've changed, but not sure it really matters

This revision is now accepted and ready to land.Thu, Oct 3, 12:01 AM
varun added inline comments.Thu, Oct 3, 12:38 AM
lib/utils/neynar-client.js
172

we can continue this discussion on linear tomorrow https://linear.app/comm/issue/ENG-9479/reduce-number-of-neynarclient-api-calls-in-join-thread

176

fair. i'll just check if (cachedEntry)

Closed by commit rCOMMf42a0f28297b: [lib] cache led channels response from neynar (authored by varun). · Explain WhyThu, Oct 3, 12:42 AM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMMf42a0f28297b: [lib] cache led channels response from neynar.

Revision Contents
Changeset List

PathSize
lib/
utils/
33 lines

Diff 44848

View Options

lib/utils/neynar-client.js

Loading...