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.

Sign up for GitHub

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

Jump to bottom

Add dry run for backfill #45062

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Add dry run for backfill #45062

wants to merge 25 commits into from

Conversation

prabhusneha
Copy link
Contributor

Closes #44395

Response:
image

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Dec 18, 2024
@prabhusneha prabhusneha marked this pull request as ready for review December 19, 2024 13:55
@phanikumv phanikumv requested a review from dstandish December 19, 2024 13:59
pierrejeambrun
pierrejeambrun reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As discussed with daniel, maybe a separate endpoint makes more sense to avoid mixed returned type BackfillResponse | BackfillDryRunResponse on the same endpoint. That's hard to handle for clients.

@prabhusneha
Copy link
Contributor Author

As discussed with daniel, maybe a separate endpoint makes more sense to avoid mixed returned type BackfillResponse | BackfillDryRunResponse on the same endpoint. That's hard to handle for clients.

Created a separate endpoint for dry run.

pierrejeambrun
pierrejeambrun reviewed Dec 25, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Thanks, looking nice.

A few improvement suggestions.

airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/backfills.py Outdated Show resolved Hide resolved
@jscheffl
Copy link
Contributor

jscheffl commented Jan 1, 2025

Note: As PR #45312 has been merged, the code formatting rules have changed for new UI. Please rebase and re-run pre-commit checks to ensure that formatting in folder airflow/ui is adjusted.

pierrejeambrun
pierrejeambrun reviewed Jan 6, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Overall looking good.

A few suggestions.

I would wait for @dstandish approval before merging that, just to be sure that the backfill logic is correct. (It looks good to me)

airflow/models/backfill.py Outdated Show resolved Hide resolved
airflow/models/backfill.py Outdated Show resolved Hide resolved
airflow/models/backfill.py Outdated Show resolved Hide resolved
airflow/models/backfill.py Outdated Show resolved Hide resolved
pierrejeambrun
pierrejeambrun reviewed Jan 6, 2025
View reviewed changes
airflow/models/backfill.py Outdated Show resolved Hide resolved
airflow/models/backfill.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/backfills.py Outdated Show resolved Hide resolved
dstandish
dstandish approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

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

only thing i changed is to not lock the dag runs for dry run since it's not necessary.

pierrejeambrun
pierrejeambrun approved these changes Jan 10, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

airflow/models/backfill.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/backfills.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstandish dstandish dstandish approved these changes

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@bbovenzi bbovenzi bbovenzi left review comments

@uranusjr uranusjr uranusjr left review comments

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@jscheffl jscheffl Awaiting requested review from jscheffl jscheffl is a code owner

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees
No one assigned
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add robust dry run capability for backfill
6 participants
@prabhusneha @jscheffl @uranusjr @bbovenzi @pierrejeambrun @dstandish