-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Add deferrable mode in RedshiftPauseClusterOperator #28850
Changes from 4 commits
5469ad4
4dd2f17
a407e4c
a59aa0b
7508ad2
e7e1010
37c307a
1ba1df7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -801,6 +801,30 @@ jobs: | |
run: breeze ci fix-ownership | ||
if: always() | ||
|
||
tests-aws-async-provider: | ||
timeout-minutes: 50 | ||
name: "Pytest for AWS Async Provider" | ||
runs-on: "${{needs.build-info.outputs.runs-on}}" | ||
needs: [build-info, wait-for-ci-images] | ||
if: needs.build-info.outputs.run-tests == 'true' | ||
steps: | ||
- name: Cleanup repo | ||
shell: bash | ||
run: docker run -v "${GITHUB_WORKSPACE}:/workspace" -u 0:0 bash -c "rm -rf /workspace/*" | ||
- name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )" | ||
uses: actions/checkout@v3 | ||
with: | ||
persist-credentials: false | ||
- name: "Prepare breeze & CI image" | ||
uses: ./.github/actions/prepare_breeze_and_image | ||
- name: "Run AWS Async Test" | ||
run: "breeze shell \ | ||
'pip install aiobotocore>=2.1.1 && pytest /opt/airflow/tests/providers/amazon/aws/deferrable'" | ||
Comment on lines
+820
to
+822
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All other settings would not apply for this run, include collect warnings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, but I'm also not sure if that require right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess when this PR would be ready for merge it would be solved? |
||
- name: "Post Tests" | ||
uses: ./.github/actions/post_tests | ||
- name: "Fix ownership" | ||
run: breeze ci fix-ownership | ||
if: always() | ||
|
||
tests-helm: | ||
timeout-minutes: 80 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
from airflow.exceptions import AirflowException | ||
from airflow.models import BaseOperator | ||
from airflow.providers.amazon.aws.hooks.redshift_cluster import RedshiftHook | ||
from airflow.providers.amazon.aws.triggers.redshift_cluster import RedshiftClusterTrigger | ||
|
||
if TYPE_CHECKING: | ||
from airflow.utils.context import Context | ||
|
@@ -447,6 +448,7 @@ class RedshiftPauseClusterOperator(BaseOperator): | |
:param cluster_identifier: id of the AWS Redshift Cluster | ||
:param aws_conn_id: aws connection to use | ||
:param deferrable: Run operator in the deferrable mode. This mode requires an additional aiobotocore>= | ||
""" | ||
|
||
template_fields: Sequence[str] = ("cluster_identifier",) | ||
|
@@ -458,11 +460,15 @@ def __init__( | |
*, | ||
cluster_identifier: str, | ||
aws_conn_id: str = "aws_default", | ||
deferrable: bool = False, | ||
poll_interval: int = 10, | ||
Comment on lines
+463
to
+464
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Personally, I think I'd like to see all operators which implement a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the first part, I don't know offhand but @syedahsn is working on deferrable stuff on our end and might have a better answer there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any chance that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't speak for the botocore team on that, I don't have any contact with that team, so I wouldn't know any better than you on that one. I'm not aware of anything in that regard but I wouldn't really know. Syed is working on researching deferrable operators to start making the full suite of AWS operators deferrable though, so out of our team I'd say he's the closest to an expert on aiobotocore and related stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been requested for a looong time: boto/botocore#458 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think we do not have native support soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our team is currently working on implementing deferrable operators as well, and we're taking a slightly different approach. Rather than create async counterparts for all AWS hooks, we are planning to create an |
||
**kwargs, | ||
): | ||
super().__init__(**kwargs) | ||
self.cluster_identifier = cluster_identifier | ||
self.aws_conn_id = aws_conn_id | ||
self.deferrable = deferrable | ||
self.poll_interval = poll_interval | ||
# These parameters are added to address an issue with the boto3 API where the API | ||
# prematurely reports the cluster as available to receive requests. This causes the cluster | ||
# to reject initial attempts to pause the cluster despite reporting the correct state. | ||
|
@@ -472,18 +478,48 @@ def __init__( | |
def execute(self, context: Context): | ||
redshift_hook = RedshiftHook(aws_conn_id=self.aws_conn_id) | ||
|
||
while self._attempts >= 1: | ||
try: | ||
redshift_hook.get_conn().pause_cluster(ClusterIdentifier=self.cluster_identifier) | ||
return | ||
except redshift_hook.get_conn().exceptions.InvalidClusterStateFault as error: | ||
self._attempts = self._attempts - 1 | ||
|
||
if self._attempts > 0: | ||
self.log.error("Unable to pause cluster. %d attempts remaining.", self._attempts) | ||
time.sleep(self._attempt_interval) | ||
else: | ||
raise error | ||
if self.deferrable: | ||
self.defer( | ||
timeout=self.execution_timeout, | ||
trigger=RedshiftClusterTrigger( | ||
task_id=self.task_id, | ||
poll_interval=self.poll_interval, | ||
aws_conn_id=self.aws_conn_id, | ||
cluster_identifier=self.cluster_identifier, | ||
attempts=self._attempts, | ||
operation_type="pause_cluster", | ||
), | ||
method_name="execute_complete", | ||
) | ||
else: | ||
while self._attempts >= 1: | ||
try: | ||
redshift_hook.get_conn().pause_cluster(ClusterIdentifier=self.cluster_identifier) | ||
return | ||
except redshift_hook.get_conn().exceptions.InvalidClusterStateFault as error: | ||
self._attempts = self._attempts - 1 | ||
|
||
if self._attempts > 0: | ||
self.log.error("Unable to pause cluster. %d attempts remaining.", self._attempts) | ||
time.sleep(self._attempt_interval) | ||
else: | ||
raise error | ||
|
||
def execute_complete(self, context: Context, event: Any = None) -> None: | ||
""" | ||
Callback for when the trigger fires - returns immediately. | ||
Relies on trigger to throw an exception, otherwise it assumes execution was | ||
successful. | ||
""" | ||
if event: | ||
if "status" in event and event["status"] == "error": | ||
msg = f"{event['status']}: {event['message']}" | ||
raise AirflowException(msg) | ||
elif "status" in event and event["status"] == "success": | ||
self.log.info("%s completed successfully.", self.task_id) | ||
self.log.info("Paused cluster successfully") | ||
else: | ||
raise AirflowException("No event received from trigger") | ||
|
||
|
||
class RedshiftDeleteClusterOperator(BaseOperator): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it will run even if no changes in AWS Provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, what we should do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should we run only on Amazon changes, I guess @potiuk might suggest as original autor of selective check for providers