Page MenuHomePhabricator
Differential D13582

[lib] cache led channels response from neynar
ClosedPublic
Actions

Authored by varun on Oct 2 2024, 10:05 PM.
Tags
None
Referenced Files
F3514051: D13582.id44852.diff
Sun, Dec 22, 3:12 AM
F3513122: D13582.diff
Sat, Dec 21, 10:36 PM
F3512951: D13582.id44848.diff
Sat, Dec 21, 9:17 PM
Unknown Object (File)
Wed, Dec 18, 9:06 AM
Unknown Object (File)
Wed, Dec 18, 9:06 AM
Unknown Object (File)
Wed, Dec 18, 9:06 AM
Unknown Object (File)
Wed, Dec 18, 9:06 AM
Unknown Object (File)
Sat, Nov 23, 3:47 AM
View All 51 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

Harbormaster completed remote builds in B32031: Diff 44848.Oct 2 2024, 10:22 PM
varun requested review of this revision.Oct 2 2024, 10:22 PM
ashoat accepted this revision.Oct 3 2024, 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 ↗(On Diff #44848)

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 ↗(On Diff #44848)

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.Oct 3 2024, 12:01 AM
varun added inline comments.Oct 3 2024, 12:38 AM
lib/utils/neynar-client.js
172 ↗(On Diff #44848)

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 ↗(On Diff #44848)

fair. i'll just check if (cachedEntry)

Closed by commit rCOMMf42a0f28297b: [lib] cache led channels response from neynar (authored by varun). · Explain WhyOct 3 2024, 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 44852

View Options

lib/utils/neynar-client.js

Loading...