ferruzzi commented on PR #62645:
URL: https://github.com/apache/airflow/pull/62645#issuecomment-4085786921
@ashb
> I'm not thrilled with "diplicating" supervise and CallbackSubprocess.
>
>Mostly because there's more code there than I'd like and having just one
thing for "run user code" works be nicer.
>
>Did you look at and decide against a single code bath for supervising both
ExecutorWorkloads?
Most of `supervise` is pretty heavily tied into the Task Instance lifecycle,
tracking and updating TI states and that isn't needed by callbacks.
Currently, callbacks don't support the Client and messaging (that's next) which
means supervise will be littered with `if isinstance()` blocks directing
callbacks away from features they don't support yet.
I can do a _little_ bit more to unify them right now (make
_configure_logging support client-None, for example), and the two paths will
start merging naturally as callbacks get more features. I hadn't intended to
add heartbeating to the callbacks, and supervise handles a lot of the TI state
machine which isn't relevant to the callbacks either. I'm not entirely sure
how that will play out if we completely merge the two, but we can figure
something out.
If you want a unified entrypoint, how do you feel about a new entrypoint
like:
```
def supervise_workload(workload, team_conf)
if isinstance(workload, workloads.ExecuteTask):
supervise(ti=workload.ti, dag_rel_path=..., token=..., server=...,
log_path=...)
elif isinstance(workload, workloads.ExecuteCallback):
supervise_callback(id=workload.callback.id, callback_path=..., ...)
```
rename the existing `supervise` to `supervise_task`, and replace all/most of
the places that call it directly to instead go through the new helper?
--
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]
