-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[prebuilds] no prebuilds for inactive repos #9936
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
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 |
---|---|---|
|
@@ -195,6 +195,11 @@ export class PrebuildManager { | |
prebuild.error = | ||
"Project is inactive. Please start a new workspace for this project to re-enable prebuilds."; | ||
await this.workspaceDB.trace({ span }).storePrebuiltWorkspace(prebuild); | ||
} else if (!project && (await this.shouldSkipInactiveRepository({ span }, cloneURL))) { | ||
prebuild.state = "aborted"; | ||
prebuild.error = | ||
"Repository is inactive. Please create a project for this repository to re-enable prebuilds."; | ||
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. Nit: I think this isn't totally accurate (I agree that we want users to create a Project, but the minimal fix is actually just to open a new workspace for that repository). However, since this error isn't actually shown anywhere in the UI, users can never see it. 😅 So, for something only a Gitpod admin can see while looking into the DB, that's perfect. 💯 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 know it's not technically correct, but it is what we would like people to do.😇 |
||
await this.workspaceDB.trace({ span }).storePrebuiltWorkspace(prebuild); | ||
} else { | ||
span.setTag("starting", true); | ||
const projectEnvVars = await projectEnvVarsPromise; | ||
|
@@ -356,4 +361,18 @@ export class PrebuildManager { | |
const inactiveProjectTime = 1000 * 60 * 60 * 24 * 7 * 1; // 1 week | ||
return now - lastUse > inactiveProjectTime; | ||
} | ||
|
||
private async shouldSkipInactiveRepository(ctx: TraceContext, cloneURL: string): Promise<boolean> { | ||
const span = TraceContext.startSpan("shouldSkipInactiveRepository", ctx); | ||
try { | ||
return ( | ||
(await this.workspaceDB | ||
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. Q: Would it make sense to add some caching here? Just in-memory, time based eviction – let's say 1d. Thinking out loud:
|
||
.trace({ span }) | ||
.getWorkspaceCountByCloneURL(cloneURL, 7 /* last week */, "regular")) === 0 | ||
); | ||
} catch (error) { | ||
log.error("cannot compute activity for repository", { cloneURL }, error); | ||
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.
Wow, many thanks for this clever fix! 👀
My only worry here is, how do we know that this query on all
d_b_workspace
entries won't be too slow/expensive? (Especially since we'll run it on every received webhook, which seems very frequent.)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.
That is a good point. All I did is running the query below against the prod failovwer DB:
Note, that it is only running for those webhooks where we don't find a project but still it could be too much. Any ideas how to move forward?
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 what EXPLAIN has to say:

So there's a key on
creationTime
which narrows the query to all workspaces of the past seven days.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.
Aha, that totally addresses the performance concern then, right? (Or, do we need to somehow test/qualify this further?)
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.
@svenefftinge, the number of rows sent is also a perf issue sometimes.
If we're just checking for
count === 0
down below, we could start out with alimit
ed version.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 expect typeorm to translate to count(*) when using
getCount
. Doesn't it do that?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.
👍🏻
https://github.com/typeorm/typeorm/blob/62518ae1226f22b2f230afa615532c92f1544f01/src/query-builder/SelectQueryBuilder.ts#L2849