Re: [PR] add log for running callback [airflow]
eladkal merged PR #38892: URL: https://github.com/apache/airflow/pull/38892 -- 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
Re: [PR] add log for running callback [airflow]
romsharon98 commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1566141583
##
airflow/models/taskinstance.py:
##
@@ -1213,10 +1213,11 @@ def _run_finished_callback(
if callbacks:
callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
for callback in callbacks:
+callback_name = qualname(callback).split(".")[-1]
try:
+log.info("Executing %s callback", callback_name)
Review Comment:
> Also since we only use the last component, we can just use
`callback.__name__` for the same effect (or just `callback`). I know this is
inherited from existing code.
changing to `callback.__name__` works.
just to let you know, changing to `callback` didn't work:
https://github.com/apache/airflow/assets/33751805/e3638098-2015-434b-b7a4-8ad61c0af005;>
--
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
Re: [PR] add log for running callback [airflow]
romsharon98 commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1566141583
##
airflow/models/taskinstance.py:
##
@@ -1213,10 +1213,11 @@ def _run_finished_callback(
if callbacks:
callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
for callback in callbacks:
+callback_name = qualname(callback).split(".")[-1]
try:
+log.info("Executing %s callback", callback_name)
Review Comment:
> Also since we only use the last component, we can just use
`callback.__name__` for the same effect (or just `callback`). I know this is
inherited from existing code.
when changing to `callback.__name__` works.
just to let you know, changing to `callback` didn't work:
https://github.com/apache/airflow/assets/33751805/e3638098-2015-434b-b7a4-8ad61c0af005;>
--
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
Re: [PR] add log for running callback [airflow]
uranusjr commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1562115669
##
airflow/models/taskinstance.py:
##
@@ -1213,10 +1213,11 @@ def _run_finished_callback(
if callbacks:
callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
for callback in callbacks:
+callback_name = qualname(callback).split(".")[-1]
try:
+log.info("Executing %s callback", callback_name)
Review Comment:
Also since we only use the last component, we can just use
`callback.__name__` for the same effect (or just `callback`). I know this is
inherited from existing code.
--
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
Re: [PR] add log for running callback [airflow]
uranusjr commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1562114016
##
airflow/models/taskinstance.py:
##
@@ -1213,10 +1213,11 @@ def _run_finished_callback(
if callbacks:
callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
for callback in callbacks:
+callback_name = qualname(callback).split(".")[-1]
try:
+log.info("Executing %s callback", callback_name)
Review Comment:
This should be put outside of the try block.
--
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
Re: [PR] add log for running callback [airflow]
romsharon98 commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1559532674
##
tests/models/test_taskinstance.py:
##
@@ -2856,8 +2856,9 @@ def on_execute_callable(context):
],
)
@patch("logging.Logger.exception")
+@patch("logging.Logger.info")
Review Comment:
Thanks for notice it, I changed it.
--
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
Re: [PR] add log for running callback [airflow]
Taragolis commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1559467638
##
tests/models/test_taskinstance.py:
##
@@ -2856,8 +2856,9 @@ def on_execute_callable(context):
],
)
@patch("logging.Logger.exception")
+@patch("logging.Logger.info")
Review Comment:
And in general this test case have some drawbacks.
E.g. `called` set only in the beginning to False, after that callback
executed twice in the loop.
So assign `called = completed = False` also should be call into the
beginning of the loop,
--
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
Re: [PR] add log for running callback [airflow]
Taragolis commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1559456921
##
tests/models/test_taskinstance.py:
##
@@ -2856,8 +2856,9 @@ def on_execute_callable(context):
],
)
@patch("logging.Logger.exception")
+@patch("logging.Logger.info")
Review Comment:
I guess it could simplify test case and do not required to repeat same
actions from the callback itself.
```python
def test_finished_callbacks_handle_and_log_exception(self, finished_state,
create_task_instance, caplog):
...
for callback_input in [[on_finish_callable], on_finish_callable]:
caplog.clear()
_run_finished_callback(callbacks=callback_input, context={})
assert called
assert not completed
assert "Executing on_finish_callable callback" in caplog.text
assert "Error when executing on_finish_callable callback" in
caplog.text
```
--
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
Re: [PR] add log for running callback [airflow]
Taragolis commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1559439376
##
tests/models/test_taskinstance.py:
##
@@ -2856,8 +2856,9 @@ def on_execute_callable(context):
],
)
@patch("logging.Logger.exception")
+@patch("logging.Logger.info")
Review Comment:
[`caplog`
fixture](https://docs.pytest.org/en/stable/how-to/logging.html#caplog-fixture)
--
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
Re: [PR] add log for running callback [airflow]
romsharon98 commented on PR #38892: URL: https://github.com/apache/airflow/pull/38892#issuecomment-2047289628 > Could we unit test it? added -- 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
Re: [PR] add log for running callback [airflow]
Taragolis commented on code in PR #38892:
URL: https://github.com/apache/airflow/pull/38892#discussion_r1559222060
##
airflow/models/taskinstance.py:
##
@@ -1214,9 +1214,10 @@ def _run_finished_callback(
callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
for callback in callbacks:
try:
+callback_name = qualname(callback).split(".")[-1]
+log.info("Executing %s callback", callback_name)
Review Comment:
```suggestion
callback_name = qualname(callback).split(".")[-1]
try:
log.info("Executing %s callback", callback_name)
```
--
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
[PR] add log for running callback [airflow]
romsharon98 opened a new pull request, #38892:
URL: https://github.com/apache/airflow/pull/38892
Today there is no log when running callbacks, in our company client want to
know if callbacks where running.
I think this should be info log and not debug log.
If you have a better message let me know
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a
newsfragment file, named `{pr_number}.significant.rst` or
`{issue_number}.significant.rst`, in
[newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
