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

[AIRFLOW-3353] Upgrade redis client. #4203

Merged
merged 1 commit into from
Nov 17, 2018
Merged

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Nov 17, 2018

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"

Description

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

Upgrade redis client to latest and rename StrictRedis to Redis.

Tests

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

Covered by existing tests.

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
Copy link

codecov-io commented Nov 17, 2018

Codecov Report

Merging #4203 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4203      +/-   ##
=========================================
+ Coverage    77.7%   77.7%   +<.01%     
=========================================
  Files         199     199              
  Lines       16315   16315              
=========================================
+ Hits        12677   12678       +1     
+ Misses       3638    3637       -1
Impacted Files Coverage Δ
airflow/configuration.py 89.05% <0%> (+0.36%) ⬆️

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 4fac6b9...465b91e. Read the comment docs.

@jmcarp jmcarp changed the title [WIP] Upgrade redis client. Upgrade redis client. Nov 17, 2018
@r39132 r39132 changed the title Upgrade redis client. [AIRFLOW-3353] Upgrade redis client. Nov 17, 2018
@r39132 r39132 merged commit ae62987 into apache:master Nov 17, 2018
@ashb
Copy link
Member

ashb commented Nov 17, 2018

I need to revert this - right now this version of Redis breaks Celery.

ashb added a commit that referenced this pull request Nov 17, 2018
ashb added a commit that referenced this pull request Nov 17, 2018
@ashb
Copy link
Member

ashb commented Nov 17, 2018

Once celery/kombu#948 or something like that is fixed in combo we can bring this change back

@r39132
Copy link
Contributor

r39132 commented Nov 17, 2018

@ashb Sorry for this.. it looked like a good change and tests were passing. Our tests didn't catch this?

@jmcarp
Copy link
Contributor Author

jmcarp commented Nov 17, 2018

Sorry! I had the same question about tests. Would it be worthwhile to add an integration test to make sure the celery executor works?

@ashb
Copy link
Member

ashb commented Nov 17, 2018

Yup, it's the fix for our hook.

Reduced integration tests (i.e. not the full suite, just a few dag tests like we do for kube) hitting celery+redis would be amazing to add!

tmiller-msft pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Nov 27, 2018
tmiller-msft pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Nov 27, 2018
elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
@ashb ashb mentioned this pull request Mar 4, 2019
4 tasks
ashb pushed a commit that referenced this pull request Mar 4, 2019
Now that Celery/Kombu have updated and work with RedisPy 3.x (they in
fact force us to use 3.2) we should re-introduce this change.
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
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

4 participants