Skip to content

Conversation

@pankajastro
Copy link
Member

@pankajastro pankajastro commented Aug 21, 2022

Add RedshiftCreateClusterSnapshotOperator to create snapshot for a redshift cluster. by default, it won't wait for the cluster to be available.


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

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Aug 21, 2022
@kazanzhy
Copy link
Contributor

Hi @pankajastro.
I think this is a good new Operator. But I think a new functionality could be added without the many separate operators.
For example, in RDS I added the parameter db_type which could be either 'instance' or 'cluster'. It gives the possibility to use a single operator for a very similar job.
It will be good even if you will add a kind of copy-paste from these PRs:

@pankajastro
Copy link
Member Author

pankajastro commented Aug 22, 2022

Hi @pankajastro. I think this is a good new Operator. But I think a new functionality could be added without the many separate operators. For example, in RDS I added the parameter db_type which could be either 'instance' or 'cluster'. It gives the possibility to use a single operator for a very similar job. It will be good even if you will add a kind of copy-paste from these PRs:

hmm, I liked the Idea but for Redshift, we already have operators like RedshiftPauseClusterOperator, RedshiftResumeClusterOperator etc. I want to add RedshiftCreateClusterSnapshotOperator and RedshiftDeleteClusterSnapshotOperator I can consolidate these two into one operator RedshiftClusterSnapshotOperator, or do you have some thoughts on which would be the existing potential operator where I can add this operator implementation?

@eladkal
Copy link
Contributor

eladkal commented Aug 22, 2022

hmm, I liked the Idea but for Redshift, we already have operators like RedshiftPauseClusterOperator, RedshiftResumeClusterOperator etc. I want to add RedshiftCreateClusterSnapshotOperator and RedshiftDeleteClusterSnapshotOperator I can consolidate these two into one operator RedshiftClusterSnapshotOperator

We can deprecate operators if there is a need.
If we can generalize and make things simpler to use I think it's better but this is a judgment call per case.
@ferruzzi @o-nikolas @vincbeck can you share your thoughts on this?

While we are on it can we address also the todo (I'm not sure what was the intent on this):

# TODO: Wrap create_cluster_snapshot
def cluster_status(self, cluster_identifier: str) -> str:

@vincbeck
Copy link
Contributor

I rather have several "small" operator than one which handles multiple use cases. I guess there can be some exceptions but here I dont mind having RedshiftCreateClusterSnapshotOperator and RedshiftDeleteClusterSnapshotOperator

@pankajastro pankajastro marked this pull request as ready for review August 23, 2022 14:57
@pankajastro pankajastro requested a review from mik-laj as a code owner August 23, 2022 14:57
Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

Looks good. A few imports and docs to update.

Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

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

Right. LGTM! Let's see what CI thinks.

Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

Thanks for addressing so quickly my comments. I just left 2 minor comments

Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

LGTM

@potiuk potiuk merged commit 994f188 into apache:main Aug 25, 2022
@pankajastro pankajastro deleted the create_redshift_cluster_snapshot branch March 9, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants