-
Notifications
You must be signed in to change notification settings - Fork 859
1177-MCV-BE Fix lazycache long lock issue #4949
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
Conversation
Size Change: +10 B (0%) Total Size: 6.87 MB ℹ️ View Unchanged
|
server/src/lib/model.ts
Outdated
@@ -144,37 +141,42 @@ export default class Model { | |||
const languages = await this.db.getAllLanguages() | |||
return languages | |||
}, | |||
DAY | |||
TimeUnits.DAY, | |||
5 * TimeUnits.SECOND, |
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.
I would not touch the 3 minute default in many cases, especially as 5 seconds locking time is too small for some endpoints that need more than 10s to resolve. This way you would just spawn new requests for the same data before the initial request has even finished, leading to wasted calculations.
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.
More than 3 min is OK for known problem areas, but less than 3 min is probably not worth the potential wasted resource consumption
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.
I only dropped below 3 min when it was obvious (some of them this one I tested on local), but you are right, there might be a temporary congestion resulting in delays. I'll up them to 3 min.
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 about this one :) This 1 minute already seemed too low for me... Up it to 3 min?
What do you think?
async findClipsWithFewVotes(
client_id: string,
locale_id: number,
count: number,
exemptFromSSRL?: boolean
): Promise<DBClip[]> {
// get cached clips for given language
const cachedClips: DBClip[] = await lazyCache(
`new-clips-per-language-${locale_id}`,
async () => {
return await this.getClipsToBeValidated(locale_id, 10000)
},
TimeUnits.MINUTE,
3 * TimeUnits.MINUTE
)()
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.
3 min is the default so you can just omit the parameter here, i.e.
async findClipsWithFewVotes(
client_id: string,
locale_id: number,
count: number,
exemptFromSSRL?: boolean
): Promise<DBClip[]> {
// get cached clips for given language
const cachedClips: DBClip[] = await lazyCache(
`new-clips-per-language-${locale_id}`,
async () => {
return await this.getClipsToBeValidated(locale_id, 10000)
},
TimeUnits.MINUTE
)()
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.
I want to be explicit on these, in case somebody changes the default.
What I mean is:
- CacheTime is 1 minute
- LockTime is 3 minutes
for this particular query. Isn't 1 minute already very small?
This is per-user hit per-locale...
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.
I see, keeping it explicit is fine. Cache time is low to not have the same clips presented during community events with a couple hundred people participating at the same time.
Internal issue
Internal main issue
The Redis based lazyCache implementation had a very long time (3 hours fixed), which prevented more quick updates.
As part of re-visiting the Redis caching in general to optimize query performance by using caching, we first fix this issue.
This adds a new lockDurationMs parameter to lazyCache, which defaults to 3 minutes. Also added some plausible lock duration to the calling functions, to be fine-tuned after fine examination of query performances.