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

14.0 imp queue job chained job #2

Open
wants to merge 2 commits into
base: 14.0
Choose a base branch
from

Conversation

nilshamerlinck
Copy link
Owner

@nilshamerlinck nilshamerlinck commented Aug 12, 2024

PR Type

Bug fix, Enhancement, Tests


Description

  • Refactored common SQL query into _get_common_dependent_jobs_query method.
  • Enhanced enqueue_waiting method to use the refactored query.
  • Introduced cancel_dependent_jobs method to handle child job cancellations.
  • Added explicit flush after job state changes to ensure child jobs are updated.
  • Added tests to verify dependent job state changes and cancellations.

Changes walkthrough 📝

Relevant files
Enhancement
job.py
Refactor and enhance job dependency handling                         

queue_job/job.py

  • Added _get_common_dependent_jobs_query method to refactor common SQL
    query.
  • Updated enqueue_waiting method to use the new query method.
  • Introduced cancel_dependent_jobs method to handle cancellation of
    child jobs.
  • +10/-2   
    Bug fix
    queue_job.py
    Ensure child job state updates and handle cancellations   

    queue_job/models/queue_job.py

  • Added explicit flush after changing job state to ensure child jobs are
    updated.
  • Added call to cancel_dependent_jobs when a job is cancelled.
  • +3/-0     
    Tests
    test_job.py
    Add tests for job dependency state changes and cancellations

    test_queue_job/tests/test_job.py

  • Added test test_button_done_enqueue_waiting_dependencies to verify
    dependent job state changes.
  • Added test test_button_cancel_dependencies to verify dependent job
    cancellations.
  • +37/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    - an explicit flush is needed or child jobs won't be updated
    - no need to forward port, this was fixed already in 16.0+
    @nilshamerlinck
    Copy link
    Owner Author

    @trobz-bot /help

    Copy link

    trobz-bot bot commented Aug 12, 2024

    Trobz Bot 🤖

    Here are the supported commands:

    ToolCommandDescription

    /describe

    /describeGenerates PR description - title, type, summary, code walkthrough and labels

    /review

    /reviewFeedback about the PR, possible issues, security concerns and more

    /improve

    /improveCode suggestions for improving the PR

    /ask

    /askAnswering free-text questions about the PR

    @nilshamerlinck
    Copy link
    Owner Author

    @trobz-bot /describe

    @trobz-bot trobz-bot bot added enhancement New feature or request Bug fix Tests labels Aug 12, 2024
    Copy link

    trobz-bot bot commented Aug 12, 2024

    PR Description updated to latest commit (189a307)

    @nilshamerlinck
    Copy link
    Owner Author

    @trobz-bot /improve

    Copy link

    trobz-bot bot commented Aug 12, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a WHERE clause to the SQL query to prevent updating unintended rows

    The SQL query in _get_common_dependent_jobs_query method lacks a WHERE clause, which
    could lead to updating all rows in the queue_job table unintentionally. It is
    crucial to specify conditions to target only the relevant jobs.

    queue_job/job.py [545-549]

     def _get_common_dependent_jobs_query(self):
         return """
             UPDATE queue_job
             SET state = %s
             FROM (
    +        WHERE job_id = %s
     
    Suggestion importance[1-10]: 9

    Why: Adding a WHERE clause is crucial to prevent unintended updates to all rows in the queue_job table, which could lead to significant data integrity issues. This suggestion addresses a potential major bug.

    9
    Enhancement
    Enhance the exception message for better error context

    The exception message in _change_job_state should provide more context about the
    error. Including the job ID or other identifiers can help in debugging.

    queue_job/models/queue_job.py [322]

    -raise ValueError("State not supported: %s" % state)
    +raise ValueError(f"State '{state}' not supported for job ID {job_.id}")
     
    Suggestion importance[1-10]: 8

    Why: Enhancing the exception message with more context, such as the job ID, can significantly aid in debugging and maintaining the code. This is a valuable improvement for error handling.

    8
    Refactor the SQL query in cancel_dependent_jobs for clarity and correctness

    The method cancel_dependent_jobs uses the same SQL query as enqueue_waiting but with
    different intended states. To avoid confusion and potential errors, it's better to
    separate the queries or parameterize the state more clearly.

    queue_job/job.py [580-582]

     def cancel_dependent_jobs(self):
    -    sql = self._get_common_dependent_jobs_query()
    -    self.env.cr.execute(sql, (CANCELLED, self.uuid, CANCELLED, WAIT_DEPENDENCIES))
    +    sql = """
    +        UPDATE queue_job
    +        SET state = %s
    +        FROM (
    +        WHERE job_id = %s AND state = %s
    +    """
    +    self.env.cr.execute(sql, (CANCELLED, self.uuid, WAIT_DEPENDENCIES))
     
    Suggestion importance[1-10]: 7

    Why: Separating the SQL queries or parameterizing the state more clearly can improve code readability and reduce the risk of errors. However, the current implementation is functional, so this is more of an enhancement.

    7
    Performance
    Remove unnecessary flush calls to improve performance

    The method _change_job_state flushes the queue.job environment after changing the
    state, which might be redundant if no significant changes are made to the database
    that require an immediate flush. Consider removing the flush to improve performance
    unless it's proven necessary.

    queue_job/models/queue_job.py [311-319]

    -record.env["queue.job"].flush()
    +# record.env["queue.job"].flush()  # Consider removing if not necessary
     
    Suggestion importance[1-10]: 6

    Why: Removing unnecessary flush calls can improve performance. However, without knowing the exact context of the flush necessity, this suggestion is more of a performance enhancement rather than a critical fix.

    6

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix enhancement New feature or request Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants