Add deferrable mode in Redshift delete cluster #30244
Conversation
boring-cyborg
bot
added
area:providers
kind:documentation
provider:amazon-aws
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
5 times, most recently
from
1d4814f
to
0a97c7b
Compare
docs/apache-airflow-providers-amazon/operators/redshift_cluster.rst
Outdated
Show resolved
Hide resolved
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
from
1e6937f
to
f2e353c
Compare
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
5 times, most recently
from
000f937
to
b648783
Compare
There was a problem hiding this comment.
Added a few more comments. A more general comment on the PR is that the way it is written means that we are forced to duplicate logic in operators to Triggers if we want to have the same functionality between deferrable and non deferrable modes. This is seen in the retry logic that's implemented in the delete_cluster method in the operator, which is now being copied over to the Trigger. I think this approach will lead to more cpmplicated, error-prone code because any change made to one will have to be copied to the other. I'm currently in the middle of writing a PR for supporting deferrable operators as well, and I took a slightly different approach. I'd love to hear what your thoughts are on how I've gone about it.
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
2 times, most recently
from
6d6c376
to
04f8c40
Compare
Rebased on top of your PR but I see you have also created a PR for the same #30870 |
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
from
a88789d
to
a20eae1
Compare
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
3 times, most recently
from
41e4bb4
to
f19ab65
Compare
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
2 times, most recently
from
8b91cf1
to
e5c4c61
Compare
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
from
dc26d94
to
b8cb678
Compare
| @cached_property | ||
| def hook(self): | ||
| return RedshiftHook(aws_conn_id=self.aws_conn_id) |
There was a problem hiding this comment.
just out of curiosity, why are you using RedshiftHook instead of RedshiftAsyncHook? (I see it used in 3 of the other 4 triggers)
There was a problem hiding this comment.
In main we have RedshiftAsyncHook in one trigger RedshiftClusterTrigger which I'll clean once this PR will be merged
https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/triggers/redshift_cluster.py can you please point me to which trigger you are looking at? Also, we decided to go with boto waiter approach and not the async hook for aws async operator #30032
There was a problem hiding this comment.
I'll remove it in #31610 PR
|
Hi @hussein-awala / @eladkal requesting a re-review on this. thank you! |
Add the deferrable param in RedshiftDeleteClusterOperator. This will allow running RedshiftDeleteClusterOperator in an async way that means we only submit a job from the worker to delete a redshift cluster then defer to the trigger for the polling and waiter for a cluster to get removed and the worker slot won't be occupied for the whole period of task execution.
pankajastro
force-pushed
the
async_redshift_delete_cluster
branch
from
a1a1b43
to
3598464
Compare
for some reason CI was green in apache#30244 despite some issue. Most probably because of wrong rebase. This PR remove unimported value to make to consistent with other and fix the main.
Add the
deferrableparam in RedshiftDeleteClusterOperator.This will allow running RedshiftDeleteClusterOperator in an async way
that means we only submit a job from the worker to delete a redshift cluster
then defer to the trigger for the polling and waiter for a cluster to get removed
and the worker slot won't be occupied for the whole period of
task execution.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.