Skip to content
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

backend: move bench.rs from common to worker crate #5253

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

uael
Copy link
Collaborator

@uael uael commented Feb 10, 2025

Move in preparation of #5220 to not loose git history on that file


Important

Move bench.rs from windmill-common to windmill-worker and update related imports and references.

  • File Relocation:
    • Move bench.rs from windmill-common to windmill-worker.
  • Imports and References:
    • Update imports in monitor.rs, result_processor.rs, and worker.rs to use windmill_worker::bench instead of windmill_common::bench.
    • Remove bench module from windmill-common/src/lib.rs and add it to windmill-worker/src/lib.rs.
  • Benchmarking:
    • Update references to BenchmarkIter and BenchmarkInfo in worker.rs, result_processor.rs, and worker_flow.rs to reflect the new module path.

This description was created by Ellipsis for 16bf861. It will automatically update as commits are pushed.

@uael uael requested a review from rubenfiszel as a code owner February 10, 2025 08:08
Copy link

Meticulous was unable to execute a test run for this PR because the most recent commit is associated with multiple PRs. To execute a test run, please try pushing up a new commit that is only associated with this PR.

Last updated for commit 16bf861. This comment will update as new commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 16bf861 in 2 minutes and 28 seconds

More details
  • Looked at 124 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_flow.rs:86
  • Draft comment:
    The update_flow_status_after_job_completion loop is very long and nested. Consider refactoring parts of its logic into smaller helper functions to improve readability and testability.
  • 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-worker/src/worker_flow.rs:217
  • Draft comment:
    This section uses multiple JSONB_SET updates to modify the flow status. Ensure that the SQL queries are optimized and consider wrapping such complex queries in a database function if possible.
  • 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-worker/src/worker_flow.rs:261
  • Draft comment:
    The computation of whether to stop early using stop_after_if is dense. Document the expected semantics and inputs for the expression evaluation to help future maintainers.
  • 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-worker/src/worker_flow.rs:370
  • Draft comment:
    The branchall handling code uses a regex capture from the job’s runnable_path. Please double-check that the format of that path is always as expected so that parsing never fails unexpectedly.
  • 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-worker/src/worker_flow.rs:401
  • Draft comment:
    There is a function needs_resume that compares the previous module’s suspend settings. Consider adding inline comments explaining its decision logic in more detail.
  • 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-worker/src/worker_flow.rs:1322
  • Draft comment:
    The use of unwrap_or(MAX_UTC) in from_now masks timing issues. Consider logging a warning when the duration is unusually long instead of silently defaulting.
  • 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.
7. backend/windmill-worker/src/worker_flow.rs:1610
  • Draft comment:
    The static BRANCHALL_INDEX_RE is created with a regex literal; ensure that the branch naming convention is properly documented to avoid mismatches.
  • 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-worker/src/worker_flow.rs:2961
  • Draft comment:
    The is_simple_modules helper returns a boolean based on many conditions. A short comment listing the criteria for simplicity could be valuable for maintainers.
  • 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-worker/src/worker_flow.rs:3125
  • Draft comment:
    In payload_from_modules, the recursive call to insert_iter_arg is used to avoid key collision. Consider adding an inline comment describing why this recursion is necessary and what the expected renaming pattern should be.
  • 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-worker/src/worker_flow.rs:3951
  • Draft comment:
    The script_to_payload function conditionally calls script_path_to_payload if no script hash is provided, using unwrap_or pattern. It may be safer to use proper error handling rather than panicking in production code.
  • 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.
11. backend/windmill-worker/src/worker_flow.rs:4014
  • Draft comment:
    The function get_previous_job_result returns the result of the previous step. Consider handling the case where the retrieved value is not in the expected format with a clearer error message.
  • 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.
12. backend/windmill-worker/src/worker_flow.rs:4028
  • Draft comment:
    In get_previous_job_result, using .unwrap_or(0) for the JSON array length may hide potential schema issues. Verify that the logic here matches the intended database design.
  • 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.
13. backend/windmill-worker/src/worker_flow.rs:205
  • Draft comment:
    The function update_flow_status_after_job_completion_internal has very complex nested logic and numerous JSONB updates. Consider refactoring it into smaller helper functions and adding inline comments for clarity.
  • 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.
14. backend/windmill-worker/src/worker_flow.rs:1339
  • Draft comment:
    In compute_bool_from_expr, the comparison of eval_timeout output against "true" and "false" is brittle. Consider a more robust boolean conversion method.
  • 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.
15. backend/windmill-worker/src/worker_flow.rs:3640
  • Draft comment:
    Dynamic eval_timeout calls in iterator and input transforms introduce potential security risks. Ensure expressions are trusted or use proper sandboxing.
  • 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.
16. backend/windmill-worker/src/worker_flow.rs:3597
  • Draft comment:
    The function is_simple_modules contains many chained conditions and cloning operations. Consider refactoring for better performance and clarity.
  • 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.

Workflow ID: wflow_2Q6nV5hId0IwOfmy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 16bf861
Status: ✅  Deploy successful!
Preview URL: https://df335cbb.windmill.pages.dev
Branch Preview URL: https://uael-move-bench-rs.windmill.pages.dev

View logs

@rubenfiszel rubenfiszel merged commit 4a744ff into main Feb 10, 2025
3 checks passed
@rubenfiszel rubenfiszel deleted the uael/move_bench_rs branch February 10, 2025 10:40
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants