Skip to content

Dataflow - add waiting for successful job cancel #11501

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
Merged
potiuk merged 1 commit into from
Nov 6, 2020
Merged

Dataflow - add waiting for successful job cancel #11501

potiuk merged 1 commit into from
Nov 6, 2020

Conversation

@TobKed
Copy link
Contributor

@TobKed TobKed commented Oct 13, 2020 •
edited
Loading

Canceling jobs can fail, it will allow user to receive information when it happens.
Customisation of the time to wait for successful state is possible as well.

originated by discussion here: #8550 (comment)

@boring-cyborg boring-cyborg bot added the provider:google Google (including GCP) related issues label Oct 13, 2020
@TobKed TobKed mentioned this pull request Oct 13, 2020
Merged
6 tasks
@TobKed TobKed force-pushed the dataflow-wait-for-cancel-to-success branch from 5d484a0 to 14d03ba Compare October 13, 2020 16:25
@TobKed TobKed mentioned this pull request Oct 13, 2020
Merged
@potiuk
Copy link
Member

potiuk commented Oct 13, 2020

Hey @TobKed. Can you please rebase this one to the latest master. We fixed (hopefully) a problem with queues of jobs for GitHub actions and I think when you rebase, it should run much faster (more info on devlist shortly).

@TobKed TobKed force-pushed the dataflow-wait-for-cancel-to-success branch 3 times, most recently from 639a4b7 to bc9bbce Compare October 19, 2020 09:20
@TobKed TobKed changed the title [WIP] Dataflow - add waiting for successful job cancel [dependent on https://github.com/apache/airflow/pull/8550] Dataflow - add waiting for successful job cancel Oct 19, 2020
@TobKed TobKed marked this pull request as ready for review October 19, 2020 09:22
mik-laj
mik-laj reviewed Oct 20, 2020
View reviewed changes
airflow/providers/google/cloud/hooks/dataflow.py Outdated
Copy link
Member

@mik-laj mik-laj Oct 20, 2020

Choose a reason for hiding this comment

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

What other type of field do you envision here? The method signature uses ʻint. Did you want to support Optional[Int]` here?

Copy link
Contributor Author

@TobKed TobKed Oct 22, 2020

Choose a reason for hiding this comment

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

Yes. I updated type annotations to Optional[Int]. Thank you for pointing this out.

@TobKed TobKed force-pushed the dataflow-wait-for-cancel-to-success branch 3 times, most recently from dcad8ca to 35fc1f7 Compare October 23, 2020 11:07
@TobKed TobKed requested a review from mik-laj October 23, 2020 11:41
@TobKed TobKed force-pushed the dataflow-wait-for-cancel-to-success branch from 35fc1f7 to af7dfda Compare October 30, 2020 14:55
@TobKed
Copy link
Contributor Author

TobKed commented Oct 31, 2020

PTAL @aaltay @kennknowles

@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebase your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

@TobKed TobKed force-pushed the dataflow-wait-for-cancel-to-success branch 2 times, most recently from 9ad315b to 7254350 Compare November 4, 2020 18:33
@TobKed TobKed force-pushed the dataflow-wait-for-cancel-to-success branch from 7254350 to d9a6865 Compare November 4, 2020 22:10
@github-actions
Copy link

github-actions bot commented Nov 6, 2020

The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 6, 2020
@potiuk potiuk merged commit 0caec9f into apache:master Nov 6, 2020
@potiuk potiuk deleted the dataflow-wait-for-cancel-to-success branch November 6, 2020 14:14
@potiuk potiuk mentioned this pull request Nov 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@turbaszek turbaszek turbaszek approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees

No one assigned

Labels

okay to merge It's ok to merge this PR as it does not require more tests provider:google Google (including GCP) related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@TobKed @potiuk @kaxil @turbaszek @mik-laj