Skip to content
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

fix to inconsistent task instance state log message #2823

Closed
wants to merge 3 commits into from

Conversation

mchalek
Copy link
Contributor

@mchalek mchalek commented Nov 29, 2017

Dear Airflow maintainers,

This is a trivial fix to an incorrectly-formatted log message that we have been seeing in our logs. Please accept it without the usual JIRA + tests + etc due to its triviality.

Kevin

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #2823 into master will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
+ Coverage   73.84%   73.92%   +0.08%     
==========================================
  Files         159      159              
  Lines       12076    12076              
==========================================
+ Hits         8917     8927      +10     
+ Misses       3159     3149      -10
Impacted Files Coverage Δ
airflow/jobs.py 79.69% <ø> (+0.44%) ⬆️
airflow/models.py 87.31% <0%> (-0.05%) ⬇️
airflow/utils/helpers.py 56.32% <0%> (+2.87%) ⬆️
airflow/task_runner/bash_task_runner.py 100% <0%> (+6.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e7b0ab...c51c98c. Read the comment docs.

@Fokko
Copy link
Contributor

Fokko commented Nov 29, 2017

Hi @mchalek, thank you for your contribution. Would it be possible to change it the other way around. We started moving to the %s format recently. Cheers

@mchalek
Copy link
Contributor Author

mchalek commented Nov 30, 2017

hi @Fokko sure, I'll change it now. But, well not that it's my business, but isn't switching to %s kind of taking a step backwards? The python docs recommend .format() for new code: https://docs.python.org/2/library/stdtypes.html#str.format

I suspect the reasoning could be for compatibility with the log formatter that airflow is using?

@bolkedebruin
Copy link
Contributor

bolkedebruin commented Nov 30, 2017

@mchalek it doesn't say that. It says it is preferred for formatting strings (and thus not logging per se). However, if you want to use lazy logging you need to use %

Strike that. @Fokko please reconsider, this is string formatting not logging. .format should be used here imho.

@mchalek
Copy link
Contributor Author

mchalek commented Nov 30, 2017

hi @bolkedebruin thanks for clarifying. It's too bad that the logger class does not support the new-style of formatting.

@ron819
Copy link
Contributor

ron819 commented Dec 6, 2018

Is this still needed?

@mchalek
Copy link
Contributor Author

mchalek commented Dec 6, 2018

@ron819 it looks like someone else fixed it. i will close this PR.

@mchalek mchalek closed this Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants