-
Notifications
You must be signed in to change notification settings - Fork 214
feat: Flotilla scheduler and dispatcher actors #4375
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4375 +/- ##
==========================================
- Coverage 77.72% 76.73% -0.99%
==========================================
Files 844 846 +2
Lines 113446 115597 +2151
==========================================
+ Hits 88176 88704 +528
- Misses 25270 26893 +1623
🚀 New features to boost your workflow:
|
Todo, add |
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.
I think it looks good, this time had more questions rather than comments. Will approve though
} | ||
} | ||
} | ||
println!("[Scheduler] Scheduler loop complete"); |
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.
eprintln for logs instead so they don't get buffered?
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.
Once this really tripped me up during debugging
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.
Yup, will do
) -> DaftResult<()> { | ||
todo!("FLOTILLA_MS1: Implement run scheduler loop"); | ||
let mut input_exhausted = false; | ||
while !input_exhausted || scheduler.num_pending_tasks() > 0 { |
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.
Why have input_exhausted? Is it for tasks that get canceled?
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.
Input exhausted means that there won't be any new tasks submitted to the scheduler.
I can probably simplify this logic a bit more, the loop condition i was going for was essentially "keep looping until you have no more tasks to schedule"
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.
To clarify, is it possible for input_exhausted = true and scheduler.num_pending_tasks() > 0, and in what state would that be?
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.
yeah lets say we had 100 tasks in total for this query, but only 1 worker with 1 core.
All 100 tasks could have been submitted to the scheduler already, so input is exhausted, but scheduler could have 99 pending tasks because it only just scheduled 1
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.
input exhausted means the scheduler will not receive any more tasks via the task receiver channel. but pending tasks is the internal priority queue in the scheduler, which could still have tasks
Changes Made
Implement scheduler + dispatcher actors for flotilla. Including unit tests for both.
Related Issues
Checklist
docs/mkdocs.yml
navigation