[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17117664#comment-17117664 ] ASF subversion and git services commented on AIRFLOW-5615: -- Commit 8ac90b0c4fca8a89592f3939c76ecb922d93b3da in airflow's branch refs/heads/master from Ash Berlin-Taylor [ https://gitbox.apache.org/repos/asf?p=airflow.git;h=8ac90b0 ] [AIRFLOW-5615] Reduce duplicated logic around job heartbeating (#6311) Both SchedulerJob and LocalTaskJob have their own timers and decide when to call heartbeat based upon that. This makes those functions harder to follow, (and the logs more confusing) so I've moved the logic to BaseJob > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17117663#comment-17117663 ] ASF GitHub Bot commented on AIRFLOW-5615: - ashb merged pull request #6311: URL: https://github.com/apache/airflow/pull/6311 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: us...@infra.apache.org > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17112532#comment-17112532 ] ASF GitHub Bot commented on AIRFLOW-5615: - houqp commented on a change in pull request #6311: URL: https://github.com/apache/airflow/pull/6311#discussion_r428236112 ## File path: airflow/jobs/base_job.py ## @@ -147,7 +147,7 @@ def heartbeat_callback(self, session=None): Callback that is called during heartbeat. This method should be overwritten. """ -def heartbeat(self): +def heartbeat(self, only_if_necessary=False): Review comment: yes, having type hint prevents caller from passing in with a variable of wrong type by accident. it will also help with type checking within this method anywhere `only_if_necessary` is used. 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: us...@infra.apache.org > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[
https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17112080#comment-17112080
]
ASF GitHub Bot commented on AIRFLOW-5615:
-
ashb commented on a change in pull request #6311:
URL: https://github.com/apache/airflow/pull/6311#discussion_r427934971
##
File path: tests/conftest.py
##
@@ -415,3 +417,39 @@ def pytest_runtest_setup(item):
skip_quarantined_test(item)
skip_if_credential_file_missing(item)
skip_if_airflow_2_test(item)
+
+
+@pytest.fixture
+def frozen_sleep(monkeypatch):
+"""
+Use freezegun to "stub" sleep, so that it takes no time, but that
+``datetime.now()`` appears to move forwards
+
+If your module under test does ``import time`` and then ``time.sleep``::
+
+def test_something(frozen_sleep):
+my_mod.fn_under_test()
+
+
+If your module under test does ``from time import sleep`` then you will
+have to mock that sleep function directly::
+
+def test_something(frozen_sleep, monkeypatch):
+monkeypatch.setattr('my_mod.sleep', frozen_sleep)
+my_mod.fn_under_test()
+"""
+freezegun_control = None
+
+def fake_sleep(seconds):
+nonlocal freezegun_control
Review comment:
Given this is tests only I don't mind that much.
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:
us...@infra.apache.org
> BaseJob subclasses shouldn't implement own heartbeat logic
> --
>
> Key: AIRFLOW-5615
> URL: https://issues.apache.org/jira/browse/AIRFLOW-5615
> Project: Apache Airflow
> Issue Type: Improvement
> Components: core
>Affects Versions: 1.10.5
>Reporter: Ash Berlin-Taylor
>Assignee: Ash Berlin-Taylor
>Priority: Trivial
> Fix For: 2.0.0
>
>
> Both SchedulerJob and LocalTaskJob have their own timers and decide when to
> call heartbeat based upon that.
> That logic should be removed and live in BaseJob to simplify the code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[
https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17112043#comment-17112043
]
ASF GitHub Bot commented on AIRFLOW-5615:
-
kaxil commented on a change in pull request #6311:
URL: https://github.com/apache/airflow/pull/6311#discussion_r427913444
##
File path: tests/conftest.py
##
@@ -415,3 +417,39 @@ def pytest_runtest_setup(item):
skip_quarantined_test(item)
skip_if_credential_file_missing(item)
skip_if_airflow_2_test(item)
+
+
+@pytest.fixture
+def frozen_sleep(monkeypatch):
+"""
+Use freezegun to "stub" sleep, so that it takes no time, but that
+``datetime.now()`` appears to move forwards
+
+If your module under test does ``import time`` and then ``time.sleep``::
+
+def test_something(frozen_sleep):
+my_mod.fn_under_test()
+
+
+If your module under test does ``from time import sleep`` then you will
+have to mock that sleep function directly::
+
+def test_something(frozen_sleep, monkeypatch):
+monkeypatch.setattr('my_mod.sleep', frozen_sleep)
+my_mod.fn_under_test()
+"""
+freezegun_control = None
+
+def fake_sleep(seconds):
+nonlocal freezegun_control
Review comment:
`nonlocal` isn't available for Py2 so if we plan to backport it, we will
have to change 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> BaseJob subclasses shouldn't implement own heartbeat logic
> --
>
> Key: AIRFLOW-5615
> URL: https://issues.apache.org/jira/browse/AIRFLOW-5615
> Project: Apache Airflow
> Issue Type: Improvement
> Components: core
>Affects Versions: 1.10.5
>Reporter: Ash Berlin-Taylor
>Assignee: Ash Berlin-Taylor
>Priority: Trivial
> Fix For: 2.0.0
>
>
> Both SchedulerJob and LocalTaskJob have their own timers and decide when to
> call heartbeat based upon that.
> That logic should be removed and live in BaseJob to simplify the code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17112011#comment-17112011 ] ASF GitHub Bot commented on AIRFLOW-5615: - kaxil commented on pull request #6311: URL: https://github.com/apache/airflow/pull/6311#issuecomment-631376902 I don't get why CI is failing given you have rebased on latest master 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: us...@infra.apache.org > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17111996#comment-17111996 ] ASF GitHub Bot commented on AIRFLOW-5615: - ashb commented on a change in pull request #6311: URL: https://github.com/apache/airflow/pull/6311#discussion_r427883159 ## File path: airflow/jobs/base_job.py ## @@ -147,7 +147,7 @@ def heartbeat_callback(self, session=None): Callback that is called during heartbeat. This method should be overwritten. """ -def heartbeat(self): +def heartbeat(self, only_if_necessary=False): Review comment: > nitpick, would be nice to add type hint here. Is that needed given it has a default? 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: us...@infra.apache.org > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[
https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17074409#comment-17074409
]
ASF GitHub Bot commented on AIRFLOW-5615:
-
ashb commented on pull request #6311: [AIRFLOW-5615] Reduce duplicated logic
around job heartbeating
URL: https://github.com/apache/airflow/pull/6311
### Jira
- [x] https://issues.apache.org/jira/browse/AIRFLOW-5615
### Description
- [x] Both SchedulerJob and LocalTaskJob have their own timers and decide
when
to call heartbeat based upon that. THis makes those jobs harder to
follow, so I've moved the logic to BaseJob
### Tests
- [x] Tests added
### Commits
- [x] 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](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [x] 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](https://github.com/apache/airflow/blob/master/UPDATING.md) so
we can assign it to a appropriate release
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:
us...@infra.apache.org
> BaseJob subclasses shouldn't implement own heartbeat logic
> --
>
> Key: AIRFLOW-5615
> URL: https://issues.apache.org/jira/browse/AIRFLOW-5615
> Project: Apache Airflow
> Issue Type: Improvement
> Components: core
>Affects Versions: 1.10.5
>Reporter: Ash Berlin-Taylor
>Assignee: Ash Berlin-Taylor
>Priority: Trivial
> Fix For: 2.0.0
>
>
> Both SchedulerJob and LocalTaskJob have their own timers and decide when to
> call heartbeat based upon that.
> That logic should be removed and live in BaseJob to simplify the code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17073916#comment-17073916 ] ASF GitHub Bot commented on AIRFLOW-5615: - stale[bot] commented on pull request #6311: [AIRFLOW-5615] Reduce duplicated logic around job heartbeating URL: https://github.com/apache/airflow/pull/6311 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: us...@infra.apache.org > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[
https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17033725#comment-17033725
]
ASF GitHub Bot commented on AIRFLOW-5615:
-
ashb commented on pull request #6311: [AIRFLOW-5615] Reduce duplicated logic
around job heartbeating
URL: https://github.com/apache/airflow/pull/6311
### Jira
- [x] https://issues.apache.org/jira/browse/AIRFLOW-5615
### Description
- [x] Both SchedulerJob and LocalTaskJob have their own timers and decide
when
to call heartbeat based upon that. THis makes those jobs harder to
follow, so I've moved the logic to BaseJob
### Tests
- [x] Tests added
### Commits
- [x] 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](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [x] 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](https://github.com/apache/airflow/blob/master/UPDATING.md) so
we can assign it to a appropriate release
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:
us...@infra.apache.org
> BaseJob subclasses shouldn't implement own heartbeat logic
> --
>
> Key: AIRFLOW-5615
> URL: https://issues.apache.org/jira/browse/AIRFLOW-5615
> Project: Apache Airflow
> Issue Type: Improvement
> Components: core
>Affects Versions: 1.10.5
>Reporter: Ash Berlin-Taylor
>Assignee: Ash Berlin-Taylor
>Priority: Trivial
> Fix For: 2.0.0
>
>
> Both SchedulerJob and LocalTaskJob have their own timers and decide when to
> call heartbeat based upon that.
> That logic should be removed and live in BaseJob to simplify the code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[ https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17033724#comment-17033724 ] ASF GitHub Bot commented on AIRFLOW-5615: - stale[bot] commented on pull request #6311: [AIRFLOW-5615] Reduce duplicated logic around job heartbeating URL: https://github.com/apache/airflow/pull/6311 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: us...@infra.apache.org > BaseJob subclasses shouldn't implement own heartbeat logic > -- > > Key: AIRFLOW-5615 > URL: https://issues.apache.org/jira/browse/AIRFLOW-5615 > Project: Apache Airflow > Issue Type: Improvement > Components: core >Affects Versions: 1.10.5 >Reporter: Ash Berlin-Taylor >Assignee: Ash Berlin-Taylor >Priority: Trivial > Fix For: 2.0.0 > > > Both SchedulerJob and LocalTaskJob have their own timers and decide when to > call heartbeat based upon that. > That logic should be removed and live in BaseJob to simplify the code. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (AIRFLOW-5615) BaseJob subclasses shouldn't implement own heartbeat logic
[
https://issues.apache.org/jira/browse/AIRFLOW-5615?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16949812#comment-16949812
]
ASF GitHub Bot commented on AIRFLOW-5615:
-
ashb commented on pull request #6311: [AIRFLOW-5615] Reduce duplicated logic
around job heartbeating
URL: https://github.com/apache/airflow/pull/6311
### Jira
- [x] https://issues.apache.org/jira/browse/AIRFLOW-5615
### Description
- [] Both SchedulerJob and LocalTaskJob have their own timers and decide when
to call heartbeat based upon that. THis makes those jobs harder to
follow, so I've moved the logic to BaseJob
### Tests
- [x] Tests added
### Commits
- [x] 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](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [x] 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](https://github.com/apache/airflow/blob/master/UPDATING.md) so
we can assign it to a appropriate release
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:
us...@infra.apache.org
> BaseJob subclasses shouldn't implement own heartbeat logic
> --
>
> Key: AIRFLOW-5615
> URL: https://issues.apache.org/jira/browse/AIRFLOW-5615
> Project: Apache Airflow
> Issue Type: Improvement
> Components: core
>Affects Versions: 1.10.5
>Reporter: Ash Berlin-Taylor
>Assignee: Ash Berlin-Taylor
>Priority: Trivial
> Fix For: 1.10.6
>
>
> Both SchedulerJob and LocalTaskJob have their own timers and decide when to
> call heartbeat based upon that.
> That logic should be removed and live in BaseJob to simplify the code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
