andygrove commented on code in PR #1892:
URL:
https://github.com/apache/datafusion-ballista/pull/1892#discussion_r3482838304
##########
ballista/executor/src/execution_loop.rs:
##########
@@ -75,8 +81,9 @@ where
.unwrap()
.clone()
.into();
- let available_task_slots =
- Arc::new(Semaphore::new(executor_specification.task_slots as usize));
+ let available_task_slots = available_task_slots.unwrap_or_else(|| {
Review Comment:
Nice that this stays fully opt-in and the default path is untouched. I had a
couple of questions about the shared-semaphore case from the description.
First, on reporting: when two poll loops share one semaphore but poll
different schedulers, each loop reports
`available_task_slots.available_permits()` as its own `num_free_slots`. So both
schedulers can see the same free slots and each dispatch up to that many tasks.
The semaphore still bounds actual concurrent execution through `acquire_owned`,
so nothing over-runs. The second scheduler's tasks would just sit blocked
waiting for a permit. Is that the intended design, with the semaphore acting
purely as an execution throttle and the over-reporting being acceptable?
Second, on sizing: `DedicatedExecutor` further down is still sized to this
executor's `task_slots`. If a caller passes a semaphore with more permits than
`task_slots`, it would admit more concurrent tasks than the task runner has
threads for. That's probably a fine tradeoff for the caller to own.
Both might be worth a line in the doc comment, noting that a shared
semaphore is counted by each loop and that it controls admission only, not the
task runner's thread count.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
