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

Feature: Move celery.default_queue to operators.default_queue to allow re-use between executors #14699

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

leonsmith
Copy link
Contributor

@leonsmith leonsmith commented Mar 10, 2021

The default_queue config option resides in the celery section.

We already have an operators config section which would be a better place to specify a default.
This also allows other executors to re-use the default queue functionality without having to set celery specific config.

closes: #14696


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg
Copy link

boring-cyborg bot commented Mar 10, 2021

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

@turbaszek turbaszek added this to the Airflow 2.1 milestone Mar 13, 2021
Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me

@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 13, 2021
@turbaszek
Copy link
Member

@leonsmith could you please take a look at the CI issues?

@leonsmith
Copy link
Contributor Author

Sure @turbaszek might need a little assistance as they look pretty unrelated at a surface glance?
Is master always supposed to be green? I can re-base and see if any of them change

@leonsmith
Copy link
Contributor Author

leonsmith commented Mar 14, 2021

The test suit looked like it mostly passed last night (after a rebase onto master), 2 of the actions look like they got killed as they exited with a 137 code.
No idea why they have all reset this morning & its now saying it doesn't have permissions.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non blocking change request)

airflow/config_templates/default_test.cfg Outdated Show resolved Hide resolved
@leonsmith leonsmith force-pushed the master branch 2 times, most recently from 8243062 to bef1190 Compare March 18, 2021 11:45
@mik-laj
Copy link
Member

mik-laj commented Mar 31, 2021

@leonsmith Can you do a rebase? I would like to merge this change.

leonsmith and others added 3 commits March 31, 2021 08:36
Remove `default_queue` from airflow/config_templates/default_test.cfg as its set to the same value from airflow.cfg

Co-authored-by: Ash Berlin-Taylor <[email protected]>
@leonsmith
Copy link
Contributor Author

@mik-laj rebased as requested 👍

@mik-laj mik-laj removed the full tests needed We need to run full set of tests for this PR to merge label Mar 31, 2021
@kaxil kaxil merged commit 1d0c168 into apache:master Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:helm-chart Airflow Helm Chart area:Scheduler including HA (high availability) scheduler kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple default_queue from celery config section
5 participants