feat: memoize app getStatus calls #35426
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #35426 +/- ##
========================================
Coverage 59.46% 59.46%
========================================
Files 2830 2830
Lines 68579 68579
Branches 15177 15177
========================================
Hits 40778 40778
Misses 25132 25132
Partials 2669 2669
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| private clearExpired(): void { | ||
| const now = new Date(); | ||
|
|
||
| for (const appId in this.cache) { | ||
| if (this.cache[appId].expiresAt < now) { | ||
| delete this.cache[appId]; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
yea, I forgot to take into account that a workspace cannot have that many apps such that this would be required
| private readonly CACHE_TTL = 1000 * 60; | ||
|
|
||
| // We clean up the cache every 5 minutes | ||
| private readonly CLEANUP_INTERVAL = 1000 * 60 * 5; |
There was a problem hiding this comment.
give them environment variables so we can configure from outside?
Proposed changes (including videos or screenshots)
Due to the previous architecture of the apps-engine, several flows are dependent on calling the getStatus method exposed by the app.
However, after Deno runtime, these add a lot more overhead compared to simple in memory access, clog the output queue of the subprocess and are prone to errors that simply didn’t happen before (30s timeouts, subprocess unavailable, etc.)
These calls should be better handled to reduce the strain on the system.
Issue(s)
Steps to test or reproduce
Further comments
CONN-451