You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/02/25 02:06:09 UTC

[GitHub] [airflow] kaxil opened a new pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

kaxil opened a new pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532
 
 
   The policy is for **task** and not **task_instance**.
   
   Original commit that introduced this feature: https://github.com/apache/airflow/commit/9368c719fccb246dfaa396fea2c592d57aeacf7f
   
   Then it was updated in https://github.com/apache/airflow/commit/1c322b007ec24aeaf9889be44f057d38af3f6c49
   
   We already have documentation on it:
   - https://airflow.readthedocs.io/en/1.10.9/concepts.html#cluster-policy
   - https://airflow.apache.org/docs/1.10.9/concepts.html#cluster-policy
   
   ---
   Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [x] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
   - [x] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [x] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   <sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383895918
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   When the user copies the function, they'll also copy the annotation, makes the code more robust :)

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil merged pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383924936
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   Much better, sorry for the fuzz.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383884871
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   Maybe add a type annotation here?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383921528
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   Removed Type Annotation

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383920439
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   Ya it is causing issues: https://travis-ci.org/apache/airflow/jobs/654914183

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383887968
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   Can add but this function needs to be overriden by users in their `airflow_local_settings.py` - so adding type annotation here won't have any impact I feel. WDYT?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383902307
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   What version would you like/recommend:
   
   1. With TYPE_CHECKING - https://github.com/apache/airflow/pull/7532/commits/fb4ede5523cb1e0aea8bff95766feffb54d5f0a5
   2. Without TYPE_CHECKING - https://github.com/apache/airflow/pull/7532/commits/7776f8178d3b61cf003b10eccb7caa2be3f504a2
   
   With (2) I am just a bit worried that there might be a chance for cyclic dependencies.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383905306
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   I prefer 2, but if there is any chance of having cyclic dependencies we should just leave it out. The first one is a bit awkward.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7532: [AIRFLOW-XXXX] Fix outdated doc on settings.policy
URL: https://github.com/apache/airflow/pull/7532#discussion_r383925599
 
 

 ##########
 File path: airflow/settings.py
 ##########
 @@ -79,28 +79,26 @@
 json = json
 
 
-def policy(task_instance):
+def policy(task):
 
 Review comment:
   No worries :)

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services