-
Notifications
You must be signed in to change notification settings - Fork 878
fix: make workspace id change faster and limit to 100k jobs #7500
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
|
Claude finished @hugocasa's task in 1m 27s —— View job PR Review: Make Workspace ID Change Faster
SummaryThis PR makes two key improvements to the workspace ID change functionality:
Code QualityPositives:
Issues to address:
SecurityNo security concerns identified. The authorization checks remain in place. PerformanceThe new approach is faster because:
However, the Reproduction InstructionsNavigate to the Workspace Settings page (gear icon → Settings → General tab). Click the pencil/edit icon next to the "Workspace ID" field. A modal will appear with two alerts: one info alert explaining the operation may take a few minutes, and one warning alert about running jobs not being migrated. Enter a new workspace name and ID, then click "Save". If your workspace has more than 100,000 jobs, you should see an error message directing you to use the Windmill CLI instead. |
| ) | ||
| .fetch_one(&db) | ||
| .await? | ||
| .unwrap_or(0); |
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.
Performance consideration: The COUNT(*) query on v2_job without any index hint may be slow for workspaces with many jobs. Since we're just checking if it exceeds 100k, consider using an early exit approach:
| .unwrap_or(0); | |
| // Check total job count before attempting migration | |
| let job_count = sqlx::query_scalar!( | |
| "SELECT COUNT(*) FROM v2_job WHERE workspace_id = $1 LIMIT 100001", | |
| &old_id | |
| ) |
Actually, LIMIT doesn't work with COUNT(*) this way. An alternative is to use a subquery:
SELECT COUNT(*) FROM (SELECT 1 FROM v2_job WHERE workspace_id = $1 LIMIT 100001) AS tThis stops counting after 100001 rows, making the check faster for workspaces with millions of jobs.
| &old_id | ||
| ) | ||
| .execute(&mut *tx) | ||
| .await?; |
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.
Potential issue: This query has a subtle correctness concern. The logic is:
- Update
v2_job_queuerows whererunning = falseAND the job has no parent - Then update
v2_jobwhere the workspace_id was already updated inv2_job_queueORv2_job_completed
However, this means:
- Running jobs are not migrated (intentional based on the UI warning)
- Child jobs (with
parent_job IS NOT NULL) are not migrated - this could leave orphaned child job records
Is excluding child jobs intentional? If so, the frontend warning should mention this. If not, you may want to migrate child jobs along with their parents.
| .execute(&mut *tx) | ||
| .await?; | ||
|
|
||
| sqlx::query!( |
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.
Good catch adding these missing trigger types! The postgres_trigger, mqtt_trigger, gcp_trigger, and sqs_trigger tables were missing from the workspace migration. This is an important fix.
| </Alert> | ||
| <Alert type="warning" title="Running jobs and queued flow steps will not be migrated."> | ||
| Make sure that all your jobs are completed before changing the workspace ID. | ||
| </Alert> |
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.
Good UX improvement: Splitting the alerts into an info alert and a warning alert makes the messaging clearer. The warning about running jobs not being migrated is helpful for users to understand what to expect.
Deploying windmill with
|
| Latest commit: |
a342210
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e4d4a8bf.windmill.pages.dev |
| Branch Preview URL: | https://hc-make-workspace-id-change.windmill.pages.dev |
No description provided.