[AIRFLOW-4939] Simplify Code for Default Task Retries

[kaxil]

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-4939

Description

• Here are some details about my PR, including screenshots of any UI changes:

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

[XD-DENG]

LGTM
EDIT: I'll have a look at the test failure. Revoking my approval first.

revoking my approval for now

[msumit]

I wrote the exact piece of code earlier, but 1 test won't pass due to python's nature of not re-calculating function defaults again. https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

[kaxil]

huh, you are right, @msumit - Thanks, will close this PR. We are using this type of code in many places AFAIK, we should change that too.

[ashb]

Except an integer isn't mutable -- that caveat only applies to dicts and lists I thought?

[kaxil]

😬 Yeah (not sure what I was thinking) - Fixed the test.

[codecov-io]

Codecov Report

Merging #6233 into master will increase coverage by <.01%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master    #6233      +/-   ##
==========================================
+ Coverage   80.06%   80.06%   +<.01%     
==========================================
  Files         610      610              
  Lines       35261    35261              
==========================================
+ Hits        28231    28232       +1     
+ Misses       7030     7029       -1

Impacted Files	Coverage Δ
airflow/contrib/operators/pubsub_operator.py	100% <ø> (ø)	⬆️
airflow/lineage/datasets.py	84.72% <ø> (ø)	⬆️
airflow/executors/local_executor.py	82.17% <ø> (ø)	⬆️
airflow/example_dags/example_gcs_to_gcs.py	100% <ø> (ø)	⬆️
airflow/contrib/hooks/redis_hook.py	100% <ø> (ø)	⬆️
...irflow/contrib/operators/gcp_container_operator.py	100% <ø> (ø)	⬆️
airflow/contrib/hooks/ftp_hook.py	69.02% <ø> (ø)	⬆️
airflow/contrib/operators/gcp_tasks_operator.py	0% <ø> (ø)	⬆️
airflow/kubernetes/volume.py	100% <ø> (ø)	⬆️
...low/contrib/example_dags/example_winrm_operator.py	0% <ø> (ø)	⬆️
... and 413 more

---

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 d719e1f...7d1b998. Read the comment docs.

[msumit]

@ashb if that law doesn't apply on Integers, then the tests would pass, so IMO it does applies. @kaxil I don't think there is any value addition by changing the current code, in fact one would need to restart the scheduler after changing conf.

[ashb]

The reason the tests didn't pass was because:

• At parse time the value in the config was 0
• We then run the decorator to change the config, but this doesn't affect the default
• We then called the function, having the old default.

The warnings applies to a case like this:

def append_to(element, to=[]):
    to.append(element)
    return to

When calling a =append_to(1); b=append_to(2) in which case a is b -- the same default object is done. But an integer can't be mutated, it's just that our attempt to change the default value at run time in the tests doesn't work.

[ashb]

in fact one would need to restart the scheduler after changing conf

This is true with both code paths -- the airflow config file is only loaded once at process start up, so changing the config file on disk will not affect a running airflow process.

[msumit]

lgtm