-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
feat: memoize app getStatus calls #35426
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import type { AppStatus } from '../definition/AppStatus'; | ||
|
||
export class AppStatusCache { | ||
private readonly cache: Record<string, { status: AppStatus, expiresAt: Date }> = {}; | ||
private readonly CACHE_TTL = 1000 * 60; | ||
|
||
// We clean up the cache every 5 minutes | ||
private readonly CLEANUP_INTERVAL = 1000 * 60 * 5; | ||
|
||
public constructor() { | ||
setInterval(() => this.clearExpired(), this.CLEANUP_INTERVAL); | ||
} | ||
|
||
public set(appId: string, status: AppStatus): void { | ||
this.cache[appId] = { | ||
status, | ||
expiresAt: new Date(Date.now() + this.CACHE_TTL), | ||
}; | ||
} | ||
|
||
public get(appId: string): AppStatus | undefined { | ||
const cache = this.cache[appId]; | ||
|
||
if (cache && cache.expiresAt > new Date()) { | ||
return cache.status; | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
private clearExpired(): void { | ||
const now = new Date(); | ||
|
||
for (const appId in this.cache) { | ||
if (this.cache[appId].expiresAt < now) { | ||
delete this.cache[appId]; | ||
} | ||
} | ||
} | ||
} | ||
Comment on lines
+31
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this and the timer is unnecessary. if cached item expired, you are setting a fresh one anyway and returning undefined here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea, I forgot to take into account that a workspace cannot have that many apps such that this would be required |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import { AppStatusCache } from '../../src/server/AppStatusCache'; | ||
|
||
export class TestAppStatusCache extends AppStatusCache { | ||
} |
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.
give them environment variables so we can configure from outside?