-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: added 15 minutes of TTL for 1st page of all time leaderboard (@LuckySilver0021) #7447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,40 @@ | |||
| type AllTimeCacheKey = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add mode2 to the cache key
| }; | ||
|
|
||
| type CacheEntry = { | ||
| data: unknown[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the type
| type CacheEntry = { | ||
| data: unknown[]; | ||
| count: number; | ||
| timestamp: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need a timestamp and only clear using .clear()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the timestamp is removed then how will we we keep track of the 15 minutes?
or maybe What Im thinking is youre trying to say that the data will be cached until the data is changed explicitly like someone came into the leaderboard or someone overtook someone's position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the discussion in the original pr: #7415 (comment)
We know when the leaderboard is refreshed and can invalidate the cache at this time.
invalidate/empty the cache on LeaderboardDal.update
|
|
||
| class AllTimeLeaderboardCache { | ||
| private cache = new Map<string, CacheEntry>(); | ||
| private readonly TTL = 900_000; // == 15 minutes of TTL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
| const cacheKey = this.getKey(key); | ||
| const entry = this.cache.get(cacheKey); | ||
|
|
||
| if (!entry || Date.now() - entry.timestamp > this.TTL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
| try { | ||
| allTimeLeaderboardCache.clear(); | ||
| console.log("All-time leaderboard cache cleared"); | ||
| } catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can this happen?
| dailyLeaderboardsConfig, | ||
| ); | ||
| try { | ||
| allTimeLeaderboardCache.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not the correct place, this is the dailyLeaderboard.
| throw new MonkeyError(500, "Invalid page or pageSize"); | ||
| } | ||
|
|
||
| if (page === 0 && pageSize === 50 && uid === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets move the cache to the controller/leaderboard and remove unnecessary try/catch and logs
| if (e.error === 175) { | ||
| if ((e as unknown as { error: number }).error === 175) { | ||
| //QueryPlanKilled, collection was removed during the query | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverting this will scream linting errors and it wont commit the code changes, I tried fixing it but I couldnt make it.
help me with this please, but apart from that I have successfully made the requested changes.
as of now I will just update the pr except this part only becuase I couldnt commit and push the changes,
further let me know how to deal with that.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// oxlint-disable-next-line no-unsafe-member-access is telling the linter to ignore this. npm run lint-be doesn't fail for me.
| if (e.error === 175) { | ||
| if ((e as unknown as { error: number }).error === 175) { | ||
| //QueryPlanKilled, collection was removed during the query | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
Changes
backend/src/utils/all-time-leaderboard-cache.tswith 15min TTLbackend/src/dal/leaderboards.tsnow checks cache HIT/MISS firstbackend/src/types/result.tscallscache.clear()on result submitBenefits
Closes #
(@LuckySilver0021)