[AIRFLOW-3367] Run celery integration test with redis broker. #4207

jmcarp · 2018-11-18T04:44:24Z

Not sure if this is the right approach; submitting to test on travis.

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-3367
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.

Description

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

Run celery integration tests with both rabbitmq and redis brokers.

Tests

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

Patch runs existing test with multiple celery brokers.

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.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

Passes flake8

codecov-io · 2018-11-18T05:27:03Z

Codecov Report

Merging #4207 into master will decrease coverage by 62.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #4207       +/-   ##
===========================================
- Coverage   78.07%   16.05%   -62.03%     
===========================================
  Files         201      201               
  Lines       16455    16455               
===========================================
- Hits        12848     2642    -10206     
- Misses       3607    13813    +10206

Impacted Files	Coverage Δ
airflow/www_rbac/validators.py	`0% <0%> (-100%)`	⬇️
airflow/utils/log/es_task_handler.py	`0% <0%> (-100%)`	⬇️
airflow/example_dags/subdags/subdag.py	`0% <0%> (-100%)`	⬇️
airflow/www/validators.py	`0% <0%> (-100%)`	⬇️
airflow/example_dags/example_docker_operator.py	`0% <0%> (-100%)`	⬇️
airflow/example_dags/example_subdag_operator.py	`0% <0%> (-100%)`	⬇️
airflow/api/common/experimental/get_dag_runs.py	`0% <0%> (-100%)`	⬇️
airflow/www/forms.py	`0% <0%> (-100%)`	⬇️
airflow/operators/postgres_operator.py	`0% <0%> (-100%)`	⬇️
airflow/api/common/experimental/get_task.py	`0% <0%> (-100%)`	⬇️
... and 160 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 d1d612e...6f7c5cf. Read the comment docs.

Fokko · 2018-11-18T16:33:29Z

Hi @jmcarp

Thanks for picking this up. Preferably you run the tests against your own Travis instance first, instead of using the Apache Travis. The hours of Apache Travis are paid, and on your own fork is for free :-)

I don't see any changes in the tests itself. Maybe it would be nice to add a test like: Run a dag with a RedisSensor, and make sure that it will pass. So we have some actual code which communicates with Travis (and therefore also uses the RedisHook).

Fokko · 2018-11-18T16:56:50Z

Please consult with #4090 before you do any duplicate work :-)

jmcarp · 2018-11-18T17:02:47Z

@Fokko: sorry, I didn't realize this was a paid travis account. Anyway, the idea here is to test the redis backend for celery, not the redis hook or sensor, which is why I changed the test configuration but not the test code. I'll continue with this locally or on my own travis for now.

jmcarp · 2018-11-19T15:10:55Z

cc @ashb

jmcarp · 2018-11-26T02:20:31Z

Rebased and fixed the conflict with #4090. No rush, but this should be ready for review when you have time @Fokko @ashb. This will help avoid the kind of regression I introduced in #4203.

ashb

Looks good overall though!

ashb · 2018-11-26T15:51:21Z

tests/executors/test_celery_executor.py

+        test_execute_command = test_app.task(execute_command.__wrapped__)
+        patch_execute_command = mock.patch(
+            'airflow.executors.celery_executor.execute_command',
+            test_execute_command)


Could all this be packaged/encapsulated in a helper function so that the "core" bit of the test (i.e. the bits you haven't changed) remains clear?

ashb · 2018-11-26T15:52:28Z

tests/executors/test_celery_executor.py

 from airflow.executors.celery_executor import app
+from airflow.executors.celery_executor import CeleryExecutor
+from airflow.executors.celery_executor import execute_command
+from airflow.executors.celery_executor import celery_configuration
 from airflow.executors.celery_executor import CELERY_FETCH_ERR_MSG_HEADER


from airflow.executors.celery_executor import ( CELERY_FETCH_ERR_MSG_HEADER, CeleryExecutor app, celery_configuration, execute_command, )

please :)

jmcarp · 2018-11-26T20:16:07Z

Thanks for reviewing @ashb! I updated the tests like you suggested.

jmcarp · 2018-11-29T04:07:32Z

Conflicts fixed, ready for another look @ashb.

jmcarp · 2018-11-30T18:38:45Z

Ping @ashb. Hopefully this will be quick to review, since most of the diff is indenting for context managers.

tests/executors/test_celery_executor.py

Fokko · 2018-12-04T21:16:40Z

LGTM, sorry for the delayed reply. I'm currently a bit short on time :(

…#4207)

(cherry picked from commit 5710ef2)

…#4207) (cherry picked from commit 5710ef2) (cherry picked from commit 229f631)

(cherry picked from commit 5710ef2)

jmcarp force-pushed the redis-celery-integration branch from 0cf2475 to 16b3c3a Compare November 18, 2018 04:56

jmcarp force-pushed the redis-celery-integration branch 3 times, most recently from 675be46 to bd7f9cf Compare November 19, 2018 15:07

jmcarp changed the title ~~[WIP] Run celery integration test with redis broker.~~ Run celery integration test with redis broker. Nov 19, 2018

jmcarp changed the title ~~Run celery integration test with redis broker.~~ [AIRFLOW-3367] Run celery integration test with redis broker. Nov 19, 2018

jmcarp force-pushed the redis-celery-integration branch 3 times, most recently from a885f33 to ae3447d Compare November 26, 2018 02:03

jmcarp force-pushed the redis-celery-integration branch from ae3447d to ebbfab3 Compare November 26, 2018 14:57

ashb reviewed Nov 26, 2018

View reviewed changes

jmcarp force-pushed the redis-celery-integration branch from ebbfab3 to 28a6d3b Compare November 26, 2018 18:38

jmcarp force-pushed the redis-celery-integration branch 3 times, most recently from 820e6ea to 279046c Compare November 29, 2018 02:03

jmcarp force-pushed the redis-celery-integration branch from 279046c to c101055 Compare November 30, 2018 16:50

[AIRFLOW-3367] Run celery integration test with redis broker.

6f7c5cf

jmcarp force-pushed the redis-celery-integration branch from c101055 to 6f7c5cf Compare November 30, 2018 16:58

ashb reviewed Dec 1, 2018

View reviewed changes

tests/executors/test_celery_executor.py Show resolved Hide resolved

ashb approved these changes Dec 2, 2018

View reviewed changes

ashb merged commit 5710ef2 into apache:master Dec 3, 2018

elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018

[AIRFLOW-3367] Run celery integration test with redis broker. (apache…

364e67a

…#4207)

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019

[AIRFLOW-3367] Run celery integration test with redis broker. (apache…

f97a34a

…#4207)

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-3367] Run celery integration test with redis broker. (apache…

3a5da8e

…#4207)

ashb pushed a commit that referenced this pull request May 22, 2020

[AIRFLOW-3367] Run celery integration test with redis broker. (#4207)

229f631

(cherry picked from commit 5710ef2)

ashb added this to the Airflow 1.10.11 milestone May 22, 2020

kaxil pushed a commit to astronomer/airflow that referenced this pull request Jun 11, 2020

[AIRFLOW-3367] Run celery integration test with redis broker. (apache…

41eb286

…#4207) (cherry picked from commit 5710ef2) (cherry picked from commit 229f631)

kaxil pushed a commit that referenced this pull request Jul 1, 2020

[AIRFLOW-3367] Run celery integration test with redis broker. (#4207)

b2a4032

(cherry picked from commit 5710ef2)

cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021

Run celery integration test with redis broker. (apache#4207)

c7095eb

(cherry picked from commit 5710ef2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3367] Run celery integration test with redis broker. #4207

[AIRFLOW-3367] Run celery integration test with redis broker. #4207

jmcarp commented Nov 18, 2018 •

edited

Loading

codecov-io commented Nov 18, 2018 •

edited

Loading

Fokko commented Nov 18, 2018

Fokko commented Nov 18, 2018

jmcarp commented Nov 18, 2018

jmcarp commented Nov 19, 2018

jmcarp commented Nov 26, 2018

ashb left a comment

ashb Nov 26, 2018

ashb Nov 26, 2018

jmcarp commented Nov 26, 2018

jmcarp commented Nov 29, 2018

jmcarp commented Nov 30, 2018

Fokko commented Dec 4, 2018

[AIRFLOW-3367] Run celery integration test with redis broker. #4207

[AIRFLOW-3367] Run celery integration test with redis broker. #4207

Conversation

jmcarp commented Nov 18, 2018 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

codecov-io commented Nov 18, 2018 • edited Loading

Codecov Report

Fokko commented Nov 18, 2018

Fokko commented Nov 18, 2018

jmcarp commented Nov 18, 2018

jmcarp commented Nov 19, 2018

jmcarp commented Nov 26, 2018

ashb left a comment

Choose a reason for hiding this comment

ashb Nov 26, 2018

Choose a reason for hiding this comment

ashb Nov 26, 2018

Choose a reason for hiding this comment

jmcarp commented Nov 26, 2018

jmcarp commented Nov 29, 2018

jmcarp commented Nov 30, 2018

Fokko commented Dec 4, 2018

jmcarp commented Nov 18, 2018 •

edited

Loading

codecov-io commented Nov 18, 2018 •

edited

Loading