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

prevent cancelling schedule next tick + remove need of flow_status inside add_completed_job #5022

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HugoCasa
Copy link
Contributor

@HugoCasa HugoCasa commented Jan 6, 2025

Important

Prevents cancelling future scheduled jobs and refines next tick scheduling logic using a new SQL query in jobs.rs.

  • Behavior:
    • Prevents cancelling future scheduled jobs in cancel_job() by checking if scheduled_for is greater than current time.
    • Refines logic in add_completed_job() to determine when to schedule the next tick using a new SQL query.
  • SQL:
    • Adds new SQL query in .sqlx/query-e4e87539ae18f7e5c6bd9a28d16b7527ececad87d218182ff723e6a6c43ecd50.json to check flow status conditions for scheduling.
  • Misc:
    • Imports now_from_db in jobs.rs for time comparison.

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

@HugoCasa HugoCasa marked this pull request as ready for review January 6, 2025 13:17
@HugoCasa HugoCasa requested a review from rubenfiszel as a code owner January 6, 2025 13:17
@HugoCasa HugoCasa marked this pull request as draft January 6, 2025 13:18
@HugoCasa HugoCasa requested a review from uael January 6, 2025 13:18
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 45b845d in 30 seconds

More details
  • Looked at 110 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. backend/windmill-queue/src/jobs.rs:236
  • Draft comment:
    Typo in error message: 'direcly' should be 'directly'.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The error message in the cancel_job function contains a typo. The word 'direcly' should be 'directly'. This is a minor issue but should be corrected for clarity and professionalism.
2. backend/windmill-queue/src/jobs.rs:742
  • Draft comment:
    Consider using Uuid::nil().to_string() directly in the query to avoid potential issues with string representation.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The SQL query in add_completed_job function is using a hardcoded string for Uuid::nil(). It would be better to use Uuid::nil().to_string() directly in the query to avoid potential issues with string representation.

Workflow ID: wflow_acSPto2tstr8knVY


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: 45b845d
Status: ✅  Deploy successful!
Preview URL: https://79514092.windmill.pages.dev
Branch Preview URL: https://hc-prevent-cancelling-schedu.windmill.pages.dev

View logs

@HugoCasa HugoCasa marked this pull request as ready for review January 6, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants