mobuchowski commented on a change in pull request #20443:
URL: https://github.com/apache/airflow/pull/20443#discussion_r778142221
##########
File path: docs/apache-airflow/plugins.rst
##########
@@ -268,6 +285,7 @@ definitions in Airflow.
operator_extra_links = [
S3LogLink(),
]
+ listeners = [PrintingRunningListener]
Review comment:
Generally, in pluggy, we need to register `plugin` that's a namespace
that consists of possibly multiple hookimpls, not single functions.
By passing just a list of functions, we'd later need to construct
"synthetic" namespace for it to register.
I think requiring a class, or, possibly a python module, is a good idea -
this is what pluggy docs present:
- class https://pluggy.readthedocs.io/en/stable/index.html#a-toy-example
- module https://pluggy.readthedocs.io/en/stable/index.html#the-plugin
Now, it does not need to look like
```python
listeners = [PrintingRunningListener]
```
We could require plugin authors to directly implement hookimpls in plugins,
for example
```python
class SomeListenerPlugin(AirflowPlugin):
name = 'some-plugin'
@hookimpl
def on_task_instance_running(...):
print("running")
```
or, we could require it to add single pluggy plugin - as class or module -
if for some reason we don't want them to implement multiple listener functions
in single plugin.
I agree that we should discourage trying to keep state in plugin - my tests
do not help, as I do that there :)
Changing the tests, and mentioning that directly in docs might be a good
idea.
Note, this is separate discussion from
https://github.com/apache/airflow/pull/20443#discussion_r778047132
--
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]
