ToxaZ commented on a change in pull request #6596: [AIRFLOW-6004] Untangle
Executors class to avoid cyclic imports
URL: https://github.com/apache/airflow/pull/6596#discussion_r353451761
##########
File path: airflow/executors/dask_executor.py
##########
@@ -66,20 +63,17 @@ def execute_async(self,
command: CommandType,
queue: Optional[str] = None,
executor_config: Optional[Any] = None) -> None:
- if not self.futures:
- raise AirflowException("Executor should be started first.")
+ assert self.futures, NOT_STARTED_MESSAGE
Review comment:
I'd rather check our options first. How we may avoid assertions.
1. If we need a oneliner: `if not self.futures: raise
Error(NOT_STARTED_MESSAGE)`. It is not very elegant but easy to understand.
1. Another option to consider is writing a single checker and call it wether
we need it:
```python
def _check_started(self):
if None in (self.client, self.futures):
raise AirflowError(NOT_STARTED_MESSAGE)
```
1. Or go more pythonic and introduce decorator.
```python
def check_futures(fn):
def wrapped(self):
if not self.futures:
raise AirflowError(NOT_STARTED_MESSAGE)```
return wrapped
@check_futures
def execute_async()
<...>
```
1. If we expect that some methods need some time to wait for executor start,
[tenacity](https://tenacity.readthedocs.io/en/latest) could be useful:
```python
from tenacity import retry
def raise_not_started():
raise AirflowError(NOT_STARTED_MESSAGE)
@retry(stop=stop_after_attempt(3))
def execute_async()
try:
<...>
except Exception:
raise AirflowError((NOT_STARTED_MESSAGE)
```
My vote goes to decorator solution -- either 3 or 4. Anyway, I think
assertions could be occasionally useful for internal checks only. Like you
checking all good and raise _your_ exception with meaningful message to end
user. Otherwise it looks like a dirty hack. Just a simple AssertionError would
confuse average pythonist as developer expects (and official docs clearly
states that) assertion usage in test environment only. Confusion harm would
outweight readability benefit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
