o-nikolas commented on code in PR #69989:
URL: https://github.com/apache/airflow/pull/69989#discussion_r3616456603
##########
task-sdk/src/airflow/sdk/plugins_manager.py:
##########
@@ -128,9 +128,17 @@ def integrate_macros_plugins() -> None:
)
-def integrate_listener_plugins(listener_manager: ListenerManager) -> None:
- """Add listeners from plugins."""
+def integrate_listener_plugins(listener_manager: ListenerManager, team_name:
str | None = None) -> None:
+ """
+ Add listeners from plugins to the given listener manager.
+
+ Only listeners from global plugins (``team_name is None``) and plugins
belonging
+ to ``team_name`` are registered. On the worker, ``team_name`` comes from
the
+ server-provided task instance context; filtering here is always applied
and does
+ not depend on the worker reading ``core.multi_team`` from its own
configuration.
+ """
plugins, _ = _get_plugins()
+ plugins = [plugin for plugin in plugins if plugin.team_name in (None,
team_name)]
Review Comment:
Yeah, I was trying to avoid having to depend on the `core.multi_team`
setting in workers, since users are very unreliable when setting these types of
configs in workers. But you are right about this gap, I see three ways out:
1. We just check config and this will sort of silently not work unless users
set the config properly (we have no way to tell if they have left it False
intentionally or not).
2. We update the team_name context field to be not present when
core.multi_team is off on the server side. So we have not present == multi-team
disabled, None == on but this is a global task and a string value == this is a
specific team. This does overload the field a bit and we have to ensure
everything operates well with back compat (when that context field didn't exist
at all) but I think it should work.
3. We add a new context field that indicates directly whether multi-team is
enabled on the server side. This would be a nice clear signal, but I could see
some folks argue that this is doing the job that the config option could be
doing and that it should just not work if users don't set it properly.
What are your thoughts on those options @amoghrajesh?
--
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]
