o-nikolas commented on issue #28276:
URL: https://github.com/apache/airflow/issues/28276#issuecomment-1344899475
> Should I leave this open until your proposal come through, just to keep
track of this specificity ? Maybe I can remove the AIP-51 if this is considered
out of scope ?
We'll still need some short term solution which this ticket can capture? At
least to fix what's been released so far (which I think are only the `is_local`
and `supports_ad_hoc_ti_run` fields) and then whatever we decide here will need
to be done for future PRs too.
Two approaches I see:
1. The fields that have been added/modified so far (e.g. `is_local` and
`supports_ad_hoc_ti_run`) can be added directly to the executors which don't
inherit from the base executor (e.g. `CeleryKubernetesExecutor`,
`LocalKubernetesExecutor`, etc)
1. Pros: fewer code changes; encapsulates the fix to code that will
eventually be deprecated/heavily modified.
2. Cons: only works for executors within Airflow core. Any custom 3rd
party executors which don't inherit from BaseExecutor will also break once this
code is released (**what level of support and backwards compatibility do we owe
these 3rd party executors?**)
1. All the locations which use the new fields added to the baseExecutor
(e.g. `is_local` and `supports_ad_hoc_ti_run`) should first check if the
executor object has that attribute set, and if not, use the default from the
BaseExceutor class (this is approximately how `supports_ad_hoc_ti_run` was
originally implemented)
1. Pros: Will be backwards compatible for all executors both in Airflow
core and public/3rd party executors
2. Cons: much uglier code and more brittle to maintain and eventually
deprecate
WDYT @pierrejeambrun? (also cc: @potiuk @eladkal)
--
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]
