-
Notifications
You must be signed in to change notification settings - Fork 164
Allow for optional jobs not interfering which benchmark completion #2342
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
Allow for optional jobs not interfering which benchmark completion #2342
Conversation
74185e7 to
3e6b6cb
Compare
I didn't understand this part. Once we enqueue a job into the job queue, it will always become benchmarked, right? We don't even need to modify the dequeue logic or anything. All that has to change is create some jobs with the optional flag, and change the "is request completed" logic to completely ignore optional jobs. |
|
I could be mistaken or over complicating what we want here. If a job is optional then I was thinking we don't want to take an optional job into consideration when marking a benchmark request as complete. Thus if a benchmark request is marked as |
|
When we dequeue a job, we don't look at its benchmark request, right? Except for loading some metadata from it. So the collector doesn't really care if the request is completed/failed or not, it will just dequeue the job regardless. |
|
Yeah you're right 👍 we don't, so this should just work 😄 |
3e6b6cb to
084675f
Compare
|
(it's still draft, is it ready for a review? :) ) |
Kobzol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks fine. Left some comments.
EDIT: Original comment cleared up below.
I see a problem with this;
If a benchmark request (req1) is marked as
completewhile it still has unfinished optional jobs, and a subsequent benchmark request (req2) is marked asin_progress, then any optional jobs belonging to req1 will be skipped.If this were implemented using a generic
is_optionalflag, those jobs would also be skipped, because the request would be consideredcompletebefore the optional jobs were ever picked up.The way I see to rectify this is to change how the dequeue query logic works for the ordering. Moreover
is_optionalis perhaps misleading as a term though I'm struggling to think of a better name. However I'd be interested to know your thoughts before doing so.For the ui should we just filter optional jobs out?