Skip to content

Add deferrable mode to DataprocInstantiateWorkflowTemplateOperator#28618

Merged
potiuk merged 1 commit into
apache:mainfrom
lwyszomi:dataproc_workflow_def
Feb 20, 2023
Merged

Add deferrable mode to DataprocInstantiateWorkflowTemplateOperator#28618
potiuk merged 1 commit into
apache:mainfrom
lwyszomi:dataproc_workflow_def

Conversation

@bkossakowska

Copy link
Copy Markdown
Contributor

Add deferrable mode to DataprocInstantiateWorkflowTemplateOperator


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does it make sense to extend this class from DataprocBaseTrigger instead of BaseTrigger https://github.com/apache/airflow/blob/main/airflow/providers/google/cloud/triggers/dataproc.py#L31 WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. Would it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

DataprocBaseTrigger is dedicated to e.g. DataprocSubmitJobOperator.
I changed the name to DataprocSubmitTrigger for clarity.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@VladaZakharova @bkossakowska somehow I thought this PR has merged so I created #29364 PR to do some refactoring on the trigger and address my above comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

merging anyone would cause conflict in others.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pankajastro
Okay, no problem!
Do you know when your changes for the trigger will be merged?

Comment thread airflow/providers/google/cloud/triggers/dataproc.py Outdated
@bkossakowska
bkossakowska force-pushed the dataproc_workflow_def branch 3 times, most recently from 97206ce to b6053e1 Compare January 17, 2023 06:48
@bkossakowska
bkossakowska force-pushed the dataproc_workflow_def branch 3 times, most recently from 1e3b5ef to 9ce75af Compare January 26, 2023 11:00
@bkossakowska

Copy link
Copy Markdown
Contributor Author

Hi @potiuk,
Is it possible to review and merge this PR?
Thanks!

@potiuk

potiuk commented Jan 30, 2023

Copy link
Copy Markdown
Member

Hi @potiuk, Is it possible to review and merge this PR? Thanks!

Yes. But after I merged the previous one, conflicts need to be resolved here :)

@bkossakowska
bkossakowska force-pushed the dataproc_workflow_def branch from 2154761 to 309944f Compare January 30, 2023 13:46
@bkossakowska

Copy link
Copy Markdown
Contributor Author

@potiuk
Done

@VladaZakharova

Copy link
Copy Markdown
Contributor

Hi!
@pankajastro @potiuk
Can you please take a look on the changes? Thanks!

@pankajastro pankajastro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants