New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change logLevel as debug for XCOM returned value message #19378
Change logLevel as debug for XCOM returned value message #19378
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be more careful with removing the log entry for Python operator which many people can rely on so conditional printing is much better, also unnecesary str() in level operator needs to be removed.
I suggest to split this change into two - one for Python operator (which is part of airlfow core package) and second for level one (which is in the google provider).
@potiuk I plan to do two things as follows.
|
This reverts commit 2835b38.
switch this PR to Draft due to the necessity to correct the related documents and write test specifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one NIT: for the dosctring (thinking about future self wondering why this parameter is here).
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
One quick question! @task
def my_task(): # Guess it is `_PythonDecoratedOperator`
# ... process
return large_data
with DAG(...) as dag:
data = my_task() # It will make a huge log messages on Airflow UI
my_second_task(data) so, I think additional work will be required in a separate PR for TaskFlow API users. airflow/airflow/decorators/python.py Lines 49 to 59 in a9772cf
|
Yes we should do something for this but separately. Maybe something like (just a random idea) @task(..., log_return_value="DEBUG")
def my_task():
...
return large_data |
@uranusjr |
Awesome work, congrats on your first merged pull request! |
I split the task API change into another issue (referenced above) |
related: #18264
The current Airflow version (2.x.x) supports Custom XCom Backends. and It means that the data from XCom could be larger than we think. (e.g. over
100GB
)However,
PythonOperator
andLevelDBOperator
will try to show all of those data from XCom as a log message. This issue can cause web issues such as breaking the web page (due to browser memory issue or rendering speed issue). and It also will make it difficult to see the other log message from the task.This PR suggests changing the logLevel
info
todebug
, Because XCom returned value can be treated `debug purpose^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.