feat(cli): add queues, workers and worker-groups commands #4439
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to aeb3cf3 in 1 minute and 27 seconds
More details
- Looked at
1648lines of code in23files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. backend/src/monitor.rs:1075
- Draft comment:
Thesave_metricslogic defaults totrueiflast_checkisNone, which might lead to unnecessary metric saving. Consider handling theNonecase more explicitly or initializinglast_checkwith a more appropriate default. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential inefficiency in the logic that defaultssave_metricstotruewhenlast_checkisNone. This could indeed lead to unnecessary operations if the intention was to avoid saving metrics in such cases. The comment is relevant to the changes made in the diff and suggests a possible improvement.
The comment assumes that saving metrics whenlast_checkisNoneis unnecessary, but without more context, it's unclear if this behavior is actually problematic. The current logic might be intentional to ensure metrics are saved at least once if no previous check exists.
The logic of defaulting totruewhenlast_checkisNonecould be intentional, but the comment raises a valid point about potential inefficiency. It's worth considering if this behavior aligns with the intended functionality.
The comment is valid as it highlights a potential inefficiency in the logic that defaultssave_metricstotruewhenlast_checkisNone. It is relevant to the changes made in the diff and suggests a possible improvement.
2. backend/windmill-api/src/configs.rs:47
- Draft comment:
Consider optimizing the removal of the 'worker__' prefix from config names by handling it at the point of data retrieval or in a single pass to improve efficiency. - Reason this comment was not posted:
Confidence changes required:50%
Inbackend/windmill-api/src/configs.rs, thelist_worker_groupsfunction modifies thenamefield ofConfigby removing the 'worker__' prefix. This is done in a loop, which is fine, but it would be more efficient to handle this transformation in a single pass or at the point of data retrieval if possible. This would reduce the need for iterating over the data multiple times.
3. backend/windmill-api/src/jobs.rs:5106
- Draft comment:
Consider using an integer type forhorizon_secsin the SQL query instead ofFloat8to avoid unnecessary type conversion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to address a potential inefficiency in the code by suggesting a type change to avoid unnecessary conversion. The SQL functionmake_intervalmight indeed expect an integer, and usingf64could be unnecessary. The comment is actionable and suggests a clear code improvement.
I might be missing the exact requirements of themake_intervalfunction and whether it strictly requires an integer. The current implementation might work as intended, and the conversion tof64might not cause any issues.
The suggestion to use an integer type seems reasonable and could improve code efficiency by avoiding unnecessary type conversion. It's worth considering this change if it aligns with the SQL function's requirements.
Keep the comment as it suggests a clear and actionable improvement to the code by recommending a type change to avoid unnecessary conversion.
4. backend/windmill-api/src/workers.rs:92
- Draft comment:
Ensure thatoccupancy_rate_5mandoccupancy_rate_30mfields are correctly populated in the database to avoid runtime errors when fetching worker pings. - Reason this comment was not posted:
Confidence changes required:50%
Inbackend/windmill-api/src/workers.rs, thelist_worker_pingsfunction fetches data from theworker_pingtable. The query includesoccupancy_rate_5mandoccupancy_rate_30m, which are new fields. Ensure that these fields are correctly populated in the database to avoid runtime errors.
Workflow ID: wflow_rRYxmVsJVe0iJMgy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Deploying windmill with
|
| Latest commit: |
eeefdf5
|
| Status: | ⚡️ Build in progress... |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on e24dc6a in 41 seconds
More details
- Looked at
114lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_p9JjFKvtFH7ghuTN
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
cli/workers.ts
Outdated
| } else { | ||
|
|
||
| new Table() | ||
| .header(["Worker ID", "Host", "Queues", "Jobs", "Occupancy rate 5m/30m/ever)", "Last job", "Last Ping"]) |
There was a problem hiding this comment.
The header 'Occupancy rate 5m/30m/ever)' has a misplaced closing parenthesis. It should be 'Occupancy rate (5m/30m/ever)'.
Suggested change
| .header(["Worker ID", "Host", "Queues", "Jobs", "Occupancy rate 5m/30m/ever)", "Last job", "Last Ping"]) | |
| .header(["Worker ID", "Host", "Queues", "Jobs", "Occupancy rate (5m/30m/ever)", "Last job", "Last Ping"]) |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0c7e73b in 28 seconds
More details
- Looked at
950lines of code in34files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/worker.rs:1374
- Draft comment:
The functionupdate_occupancy_metricsis correctly refactored to encapsulate the logic for updating occupancy metrics. This improves code readability and maintainability by separating concerns. - Reason this comment was not posted:
Confidence changes required:0%
The functionupdate_occupancy_metricsis defined incommon.rsand used inworker.rs. The function is correctly refactored to encapsulate the logic for updating occupancy metrics. This refactoring improves code readability and maintainability by separating concerns. The function is used inrun_workerto update the worker's occupancy metrics, which is consistent with its intended purpose. The refactoring does not introduce any logical errors or performance issues.
Workflow ID: wflow_ljZVvuYbDSZQIomI
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d94b803 in 27 seconds
More details
- Looked at
810lines of code in32files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. backend/src/ee.rs:7
- Draft comment:
Consider using a custom error type to indicate that this feature is not available in the community edition. This would make error handling more consistent and informative. - Reason this comment was not posted:
Confidence changes required:50%
The code inbackend/src/ee.rsandbackend/windmill-api/src/ee.rsusesanyhow::anyhow!for error handling, which is fine for general errors. However, in the context of enterprise features, it might be more appropriate to use a custom error type that clearly indicates the feature is not available in the community edition. This would make error handling more consistent and informative.
2. backend/windmill-api/src/ee.rs:9
- Draft comment:
Consider using a custom error type to indicate that this feature is not available in the community edition. This would make error handling more consistent and informative. - Reason this comment was not posted:
Confidence changes required:50%
The code inbackend/src/ee.rsandbackend/windmill-api/src/ee.rsusesanyhow::anyhow!for error handling, which is fine for general errors. However, in the context of enterprise features, it might be more appropriate to use a custom error type that clearly indicates the feature is not available in the community edition. This would make error handling more consistent and informative.
Workflow ID: wflow_eZ02RZqRBBrpdMTY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2815221 in 54 seconds
More details
- Looked at
1148lines of code in14files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. backend/windmill-worker/src/deno_executor.rs:8
- Draft comment:
Thehandle_childfunction is imported fromhandle_childmodule here. Ensure consistent import paths across the codebase for better maintainability. - Reason this comment was not posted:
Confidence changes required:50%
Thehandle_childfunction is used in multiple files, but the import path is inconsistent. It should be imported fromhandle_childmodule consistently.
2. backend/windmill-worker/src/graphql_executor.rs:15
- Draft comment:
Therun_future_with_polling_update_job_pollerfunction is imported fromhandle_childmodule here. Ensure consistent import paths across the codebase for better maintainability. - Reason this comment was not posted:
Confidence changes required:50%
Therun_future_with_polling_update_job_pollerfunction is used in multiple files, but the import path is inconsistent. It should be imported fromhandle_childmodule consistently.
3. backend/windmill-worker/src/js_eval.rs:43
- Draft comment:
Therun_future_with_polling_update_job_pollerfunction is imported fromhandle_childmodule here. Ensure consistent import paths across the codebase for better maintainability. - Reason this comment was not posted:
Confidence changes required:50%
Therun_future_with_polling_update_job_pollerfunction is used in multiple files, but the import path is inconsistent. It should be imported fromhandle_childmodule consistently.
4. backend/windmill-worker/src/mssql_executor.rs:19
- Draft comment:
Therun_future_with_polling_update_job_pollerfunction is imported fromhandle_childmodule here. Ensure consistent import paths across the codebase for better maintainability. - Reason this comment was not posted:
Confidence changes required:50%
Therun_future_with_polling_update_job_pollerfunction is used in multiple files, but the import path is inconsistent. It should be imported fromhandle_childmodule consistently.
Workflow ID: wflow_kWakeWJEKwfElFUA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d96b4ab in 53 seconds
More details
- Looked at
957lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. backend/windmill-worker/src/handle_child.rs:577
- Draft comment:
Usingunwraphere can cause a panic if the child process does not have a handle to stdout. Consider handling this case more gracefully, perhaps by returning a Result or usingexpectwith a clear error message. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. backend/windmill-worker/src/handle_child.rs:582
- Draft comment:
Usingunwraphere can cause a panic if the child process does not have a handle to stderr. Consider handling this case more gracefully, perhaps by returning a Result or usingexpectwith a clear error message. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. backend/windmill-worker/src/handle_child.rs:595
- Draft comment:
Usingunwrap_or(0)here can lead to unexpected behavior ifpoll_next_linefails. Consider handling this case explicitly to avoid potential issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential issue with error handling in thelines_to_streamfunction. The use ofunwrap_or(0)could indeed mask errors, leading to unexpected behavior. This is a valid concern as it relates to error handling, which is crucial in robust software development.
The comment assumes that the error handling is inadequate without considering the context in whichlines_to_streamis used. It's possible that the surrounding code handles errors appropriately, making this comment less relevant.
Even if the surrounding code handles errors, it's generally good practice to handle errors explicitly where they occur to avoid unexpected behavior. The comment highlights a potential improvement in error handling.
The comment raises a valid point about error handling in thelines_to_streamfunction. It suggests an improvement that could prevent unexpected behavior, making it a useful comment to keep.
4. backend/windmill-worker/src/job_logger.rs:322
- Draft comment:
Usingunwrap_or(0)here can lead to unexpected behavior if thechar_indicesiterator is empty. Consider handling this case explicitly to avoid potential issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The use ofunwrap_or(0)is a common pattern to provide a default value when an option isNone. In this context, it seems to be used to handle the case where the iterator is empty, which is a valid use case. The comment suggests that this might lead to unexpected behavior, but without specific evidence of an issue, it seems speculative.
The comment might be pointing out a potential edge case that could lead to a bug, but without specific evidence or examples, it's hard to determine if it's a real issue.
While the comment is speculative, it does raise a valid point about potential edge cases. However, without concrete evidence of an issue, it might not be necessary to address it immediately.
The comment is speculative and does not provide strong evidence of an issue. It should be removed unless there is a clear indication of a problem.
Workflow ID: wflow_sOFVao50a6lKMioH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on eeefdf5 in 52 seconds
More details
- Looked at
2167lines of code in30files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. cli/workers.ts:69
- Draft comment:
There's an extra parenthesis in the header text for occupancy rate. It should be removed for consistency.
.header(["Worker ID", "Host", "Queues", "Jobs", "Occupancy rate 15s/5m/30m/ever", "Last job", "Last Ping"])
- Reason this comment was not posted:
Confidence changes required:50%
The code incli/workers.tsandfrontend/src/routes/(root)/(logged)/workers/+page.svelteis related to displaying worker information, including occupancy rates. The occupancy rate is displayed in both the CLI and frontend, and the format is consistent. However, there is a minor inconsistency in the header text for occupancy rate in the CLI output, which includes an extra parenthesis. This should be corrected for consistency and clarity.
Workflow ID: wflow_bXYXsRZRWrUYy9Z3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Add CLI commands for managing queues, workers, and worker groups with corresponding backend and frontend support.
queues,workers, andworker-groupscommands incli/main.ts.displayQueuesincli/queues.tsto list queue metrics.displayWorkersincli/workers.tsto list workers by group.displayWorkerGroupsincli/worker_groups.tsto manage worker group configs./jobs/completed/count_by_taginopenapi.yamlfor job count by tag.worker_pingtable withoccupancy_rate_5mandoccupancy_rate_30minmigrations/20240924213010_add_last_occupancy_rates.up.sql.list_worker_pingsinworkers.rsto include new occupancy rates.+page.svelteto display worker groups and metrics..sqlxfiles for metrics and worker updates.instance.tsto handle new CLI commands and options.This description was created by
for eeefdf5. It will automatically update as commits are pushed.