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

Change Deferrable implementation for RedshiftResumeClusterOperator to follow standard #30864

Merged

Conversation

syedahsn
Copy link
Contributor

Now that #30032 is merged, we can update the deferrable implementation for RedshiftResumeClusterOperator to use custom aiobotocore waiters , as well as the async_conn to simplify deferrable implmentation, and reduce code duplication. The method used here is described in the README.md included in the initial PR.
@pankajastro @phanikumv


^ 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.

self.redshift_hook = RedshiftHook(aws_conn_id=self.aws_conn_id)
async with self.redshift_hook.async_conn as client:
waiter = self.redshift_hook.get_waiter("cluster_resumed", deferrable=True, client=client)
await waiter.wait(
Copy link
Member

Choose a reason for hiding this comment

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

Should we also think about the case where we want to log some events in trigger periodically while waiting ? similar to discussion in #30853 (comment) ?

Copy link
Member

@potiuk potiuk Apr 29, 2023

Choose a reason for hiding this comment

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

Similarly here: #30244 - it almost seems that even if the waiter implementation in botocore does not provide any way to get callback while waiitng, we could have a wrapper around the waiter that could be used across many of the Deferrable Amazon operators (this would become super useful in 2.6.0 with the triggerer logs interleaved with task logs).

Add async custom waiter support in get_waiter, and base_waiter.py
Add Deferrable mode to RedshiftCreateClusterOperator
Add RedshiftCreateClusterTrigger and unit test
Add README.md for writing Triggers for AMPP
Fix/add a few unit tests
Remove duplicate log statement
Remove duplicate docstring
@syedahsn syedahsn force-pushed the syedahsn/deferrable-redshift-resume-cluster branch from 85bf375 to 15c0a78 Compare May 10, 2023 00:55
@potiuk
Copy link
Member

potiuk commented May 17, 2023

LGTM: @o-nikolas ?

@pankajastro
Copy link
Member

@potiuk I think we are waiting for #30853 to merge then we will rebase other on top of this

@o-nikolas
Copy link
Contributor

@potiuk I think we are waiting for #30853 to merge then we will rebase other on top of this

Yup, this is correct, and I believe Pause is good to go. There was some feedback, but I think Syed has addressed it (while he is on vacation at that!).

P.S. I am on vacation starting today as well, responses will be delayed or non-existent 😁

@o-nikolas o-nikolas merged commit 44741f3 into apache:main May 24, 2023
@vandonr-amz vandonr-amz deleted the syedahsn/deferrable-redshift-resume-cluster branch May 24, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants