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

landing_job: fix ordering of jobs in queue (bug 1878666) #384

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

Conversation

zzzeid
Copy link
Contributor

@zzzeid zzzeid commented Mar 6, 2024

  • order first by priority, then by status and creation time
  • decrease job priority by 1 when it is deferred

- order first by priority, then by status and creation time
- decrease job priority by 1 when it is deferred
@zzzeid zzzeid marked this pull request as ready for review March 6, 2024 21:26
@zzzeid zzzeid requested a review from cgsheeh March 7, 2024 18:50
Comment on lines +375 to +376
if action == LandingJobAction.DEFER:
self.priority -= 1
Copy link
Member

Choose a reason for hiding this comment

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

The way this is written now, any job which has its priority lowered will always be at the back of the queue, meaning the patch effectively needs to wait until the queue is empty before it will be considered for landing. This could cause a situation where someone queues their revision for landing, it doesn't land the first time due to some transient issue, then their landing is stuck in limbo because it has a lower priority than newer revisions that are added to the queue. If the queue is empty often enough it's probably not a big deal, but worth noting here.

As an alternative we could note the last attempt time in the landing job, and filter out revisions that have been deferred for a certain duration before attempting to land again. For example we defer a landing job, and then in the job query we filter out all deferred revisions with a last attempted time within the last 15-20 minutes.

# `LandingJobStatus.SUBMITTED` jobs, higher priority items come first
# and then we order by creation time (older first).
q = q.order_by(cls.status.desc(), cls.priority.desc(), cls.created_at)
q = q.order_by(cls.priority.desc(), cls.status.desc(), cls.created_at)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this change? We already get the highest priority items with the highest priority statuses, right?

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