o-nikolas commented on code in PR #28375:
URL: https://github.com/apache/airflow/pull/28375#discussion_r1051226845
##########
airflow/sentry.py:
##########
@@ -78,14 +79,15 @@ class ConfiguredSentry(DummySentry):
def __init__(self):
"""Initialize the Sentry SDK."""
ignore_logger("airflow.task")
- executor_name = conf.get("core", "EXECUTOR")
sentry_flask = FlaskIntegration()
# LoggingIntegration is set by default.
integrations = [sentry_flask]
- if executor_name == "CeleryExecutor":
+ executor_class, _ =
ExecutorLoader.import_executor_cls(conf.get("core", "EXECUTOR"))
Review Comment:
You can use the helper method I added for this
`ExecutorLoader.import_default_executor_cls()`
##########
airflow/executors/executor_loader.py:
##########
@@ -167,10 +167,3 @@ def __load_local_kubernetes_executor(cls) -> BaseExecutor:
local_kubernetes_executor_cls =
import_string(cls.executors[LOCAL_KUBERNETES_EXECUTOR])
return local_kubernetes_executor_cls(local_executor,
kubernetes_executor)
-
-
-UNPICKLEABLE_EXECUTORS = (
Review Comment:
I wonder if we should actually keep this in for backwards compatibility
(with a comment describing that it is deprecated) :thinking: i.e. someone in
the community could be depending on this list existing. I wonder what others
think about this.
##########
airflow/executors/celery_kubernetes_executor.py:
##########
@@ -38,6 +38,7 @@ class CeleryKubernetesExecutor(LoggingMixin):
otherwise, CeleryExecutor is used.
"""
+ is_picklable: bool = True
Review Comment:
You'll also need to set a value for `supports_sentry` in this class as well
since it does not implement the `BaseExecutor` interface (see the discussion
starting [from
here](https://github.com/apache/airflow/issues/28276#issuecomment-1344899475)
for more context). You'll need to ensure both new fields are set on all the
executors which don't inherit from `BaseExecutor` (`LocalKubernetesExecutor` as
well) and add the appropriate unit tests for those.
##########
airflow/executors/base_executor.py:
##########
@@ -65,11 +65,13 @@ class BaseExecutor(LoggingMixin):
"""
supports_ad_hoc_ti_run: bool = False
+ supports_sentry: bool = False
job_id: None | int | str = None
callback_sink: BaseCallbackSink | None = None
is_local: bool = False
+ is_picklable: bool = False
Review Comment:
I think we want the default to be True, no? The previous code assumed all
executors were picklable unless it was present in the list of
`UNPICKLEABLE_EXECUTORS`. So any executor that goes through that code should be
assumed to be picklable unless it says otherwise. WDYT?
##########
airflow/executors/local_kubernetes_executor.py:
##########
@@ -38,6 +38,7 @@ class LocalKubernetesExecutor(LoggingMixin):
otherwise, LocalExecutor is used.
"""
+ is_picklable: bool = True
Review Comment:
Do we actually want this to be True? The `LocalExecutor` is not picklable
and so far we've gone with True for these only if all sub executors are True.
WDYT?
There is some context in a PR [comment
here](https://github.com/apache/airflow/pull/28288/files#r1045147675) (from
@pierrejeambrun's PR for `is_local`)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
