-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(redshift): relocating a cluster #31993
Conversation
if (props.availabilityZoneRelocation && !nodeType.startsWith('ra3')) { | ||
throw new Error(`Availability zone relocation is supported for only RA3 node types, got: ${props.nodeType}`); | ||
} | ||
|
||
this.cluster = new CfnCluster(this, 'Resource', { |
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.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Ensure that user activity logging is enabled for the Redshift cluster. This feature logs each query before it is executed on the cluster's database. To activate this, associate a Redshift Cluster Parameter Group with the enable_user_activity_logging
parameter set to true
.
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 believe this feedback is inappropriate. This implementation is for existing L2 code, and to avoid breaking changes, we should not forcefully enable enable_user_activity_logging
.
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.
Thanks. Just one question.
|
||
By using [relocation in Amazon Redshift](https://docs.aws.amazon.com/redshift/latest/mgmt/managing-cluster-recovery.html), you allow Amazon Redshift to move a cluster to another Availability Zone (AZ) without any loss of data or changes to your applications. | ||
|
||
To enable this feature, set the `availabilityZoneRelocation` property to `true` when creating the cluster. |
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.
Just to clarify, relocation cannot be enabled after a cluster has been created?
It appears from the current README that relocation cannot be enabled after a cluster is created, but the documentation suggests that it should be possible to enable it post-creation.
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.
You're absolutely right. I've updated description.
By using [relocation in Amazon Redshift](https://docs.aws.amazon.com/redshift/latest/mgmt/managing-cluster-recovery.html), you allow Amazon Redshift to move a cluster to another Availability Zone (AZ) without any loss of data or changes to your applications.
This feature can be applied to both new and existing clusters.
To enable this feature, set the `availabilityZoneRelocation` property to `true`.
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.
Thanks. LGTM.
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.
Thanks for the contribution! Overall the changes look good, just one minor nit from me for an extra test.
To back your point about relocation only being on RA3 instance types, on top of the error message you encountered, it also seems like it's buried in the fourth paragraph of that documentation page:
Amazon Redshift cluster relocation is supported for the RA3 instance types only.
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.
Thanks for the response, LGTM!
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
Pull request has been modified.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31993 +/- ##
=======================================
Coverage 77.28% 77.28%
=======================================
Files 114 114
Lines 7628 7628
Branches 1360 1360
=======================================
Hits 5895 5895
Misses 1551 1551
Partials 182 182
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
None
Reason for this change
AWS Redshift supports for configuring relocation a cluster and this feature is supported by cfn.
Description of changes
Add
availabilityZoneRelocation
toCusterProps
.Documents says that this feature is not supported for DC2 node type.
However, this feature is only supported for RA3 node type in actual.
Example implementation:
Result:
So I added this validation.
Description of how you validated changes
Add both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license