-
Notifications
You must be signed in to change notification settings - Fork 808
refactor: remove legacy database views v2_as_queue and v2_as_completed_job #6689
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
base: main
Are you sure you want to change the base?
refactor: remove legacy database views v2_as_queue and v2_as_completed_job #6689
Conversation
…d_job Signed-off-by: Ramtin Mesgari <[email protected]>
The changes in this PR will replace the view references with explicit JOINs on those tables |
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.
Important
Looks good to me! 👍
Reviewed everything up to 75e4584 in 1 minute and 41 seconds. Click for details.
- Reviewed
516
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
11
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-queue/src/jobs.rs:265
- Draft comment:
In the cancel_job recursive CTE (lines ~265–286), a fixed recursion depth (limit <500) is enforced. Ensure this limit is adequate for all use cases or consider making it configurable to prevent potential infinite recursion in cyclic job-parent chains. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-queue/src/jobs.rs:2367
- Draft comment:
The interpolate_args function uses regex-based replacements for any occurrences of "$args[...]". For large inputs or high frequency calls, consider optimizing or caching parsed values to avoid potential performance issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-queue/src/jobs.rs:2909
- Draft comment:
The macro fetch_scalar_isolated reassigns the mutable transaction variable (tx) based on its variant. This pattern can be error‐prone; consider refactoring to return a new transaction instance rather than reassigning a shared variable. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-queue/src/jobs.rs:2570
- Draft comment:
The get_result_by_id_from_running_flow_inner function uses recursion to walk up the parent chain if no result is found. Without a depth counter, a cyclic or unexpectedly deep chain could lead to stack overflow. Consider adding a recursion depth limit for safety. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-queue/src/jobs.rs:3008
- Draft comment:
The push function (starting around line 3008) is very long and complex. For maintainability and readability, consider decomposing it into smaller helper functions which encapsulate distinct parts of the job insertion logic. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-queue/src/jobs.rs:659
- Draft comment:
The register_metric function uses an RwLock to protect a HashMap of metrics. Under high concurrency this may become a contention point. Consider assessing whether this locking strategy is optimal or if alternative concurrent data structures would be beneficial. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. backend/windmill-queue/src/jobs.rs:860
- Draft comment:
In commit_completed_job, the check for whether a job has already been completed uses an existence test (unwrap_or(false)). It would be helpful to add extra logging or diagnostic details if a duplicate completion is detected, to aid debugging in production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. backend/windmill-queue/src/jobs.rs:4277
- Draft comment:
The insert_concurrency_key function builds a concurrency key using fullpath_with_workspace (or a custom key) and then performs a composite INSERT with ON CONFLICT DO NOTHING. Ensure that the underlying database schema has appropriate unique constraints on these keys to avoid duplicate insertions under high concurrency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. backend/windmill-queue/src/jobs.rs:4530
- Draft comment:
In get_same_worker_job, the SQL query uses several LEFT JOINs on job permissions and parent job information. Verify that appropriate indexes exist on the joined columns to maintain query performance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. backend/windmill-queue/src/jobs.rs:1
- Draft comment:
Overall, the refactor replaces legacy views with direct joins on underlying tables (v2_job_queue, v2_job, v2_job_status, etc.). This change appears consistent; ensure thorough integration and performance testing (including index usage and query plans) to confirm that the queries perform as expected in production environments. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
11. backend/tests/common/mod.rs:321
- Draft comment:
Minor: Consider using uppercase 'AS' for aliasing (i.e., "AS labels") to maintain consistency with the rest of the query. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about consistency, it's a very minor style issue. The SQL query will work exactly the same either way. The rules say not to make comments that are obvious or unimportant. Style consistency suggestions, unless part of a mandatory style guide, generally fall into this category. The comment does point out a real inconsistency in the code style. Having consistent casing could make the code more readable and maintainable. While code consistency is good, this is too minor of an issue to warrant a PR comment. The functionality is identical, and this kind of minor style feedback creates noise in the review process. Delete this comment as it's too minor of a style issue to be worth raising in a PR review.
Workflow ID: wflow_nHukDQKc9eQ4LoRi
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Removes legacy database views
v2_as_queue
andv2_as_completed_job
, in favour of using the underlying tables and direct table joins.The changes replace view references with explicit JOINs, also updated some SQLx type annotations to maintain compilation. All queries should return identical results as far as I was able to test and validate 🤞. But would appreciate review of the changes I've introduced, I'll be happy to adjust if I've missed anything.
References issue #6688
Signed-off-by: Ramtin Mesgari [email protected]
Important
Remove legacy database views and replace them with direct table joins in SQL queries across multiple files.
v2_as_queue
andv2_as_completed_job
.monitor.rs
,worker.rs
, andjobs.rs
.v2_job_queue
,v2_job
,v2_job_runtime
, andv2_job_status
tables.This description was created by
for 75e4584. You can customize this summary. It will automatically update as commits are pushed.