martin-g commented on code in PR #1892:
URL:
https://github.com/apache/datafusion-ballista/pull/1892#discussion_r3491574127
##########
ballista/executor/src/execution_loop.rs:
##########
Review Comment:
same here - handle AcquireError
##########
ballista/executor/src/execution_loop.rs:
##########
Review Comment:
If the externally provided Semaphore has more permits (`N`) than the
executor's task slots (`M`) then here it will request N tasks from the
Scheduler and later acquire their permits (line 144) but when trying to execute
them it will be able to actually run just `M` of them and the rest (`N-M`) will
be queued with their permits acquired.
##########
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(|| {
+ Arc::new(Semaphore::new(executor_specification.task_slots as usize))
+ });
Review Comment:
```suggestion
});
assert!(available_task_slots.available_permits() > 0 || ..., "semaphore
must have > 0 permits");
```
Passing a Semaphore with 0 permits will lead to a deadlock.
##########
ballista/executor/src/execution_loop.rs:
##########
Review Comment:
Since the `Semaphore` could be provided by a third-party now we should
handle `AcquireError` instead of panicking when the Semaphore is closed.
At the least map it to BallistaError.
--
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]
