feat(redshift): optionally reboot Clusters to apply parameter changes #22063
Conversation
|
|
github-actions
bot
added
effort/medium
| await redshift.rebootCluster({ ClusterIdentifier: clusterId }).promise(); | ||
| } catch (err) { | ||
| if ((<any>err).code === 'InvalidClusterState') { | ||
| return await executeActionForStatus(status, 30000); |
There was a problem hiding this comment.
I thought about doing some sort of backoff here, but found checking every 30 seconds to be sufficient in my testing.
dontirun
changed the title
|
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
I was waiting on #22059 to be fixed before updating. I have manually confirmed (by looking at the cluster in the integration tests) that this works. |
dontirun
force-pushed
the
feat-reboot-cluster-for-parameter-updates
branch
from
157b2ac to
52916e6
Compare
|
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
@dontirun The commented tests feel quite essential to this. I'm drafting this PR until you can get to it. Looks like a cool feature though. 🚀 |
|
@mrgrain I sadly couldn't get a few of things working with integ-tests library.
|
Hmm, we still need a way to verify the custom resource works. 🤔
The PR you've linked as been merged. Is something else missing?
Why does it need to be enabled? But yes, it's weird you get a different hash. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
6 similar comments
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts
Outdated
Show resolved
Hide resolved
| ParameterGroupName: Lazy.string({ | ||
| produce: () => { | ||
| if (!this.parameterGroup) { | ||
| throw new Error('Cannot enable reboot for parameter changes when there is no associated ClusterParameterGroup.'); |
There was a problem hiding this comment.
Putting implementation details aside I didn't think it would be a good user experience to allow for enabling a feature that does nothing.
Using a Lazy gets around an order of operations error where enabling the feature and then later adding a parameter group would trigger this error (this test case)
mergify
bot
dismissed
comcalvi’s stale review
Pull request has been modified.
packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * Whether the cluster will be rebooted when changes to the cluster's parameter group that require a restart to apply. | ||
| */ | ||
| protected rebootForParameterChangesEnabled?: boolean; |
There was a problem hiding this comment.
I don't think we need this prop, can we remove it? It seems redundant with rebootForParameterChanges?
There was a problem hiding this comment.
Related to my other comment rebootForParameterChanges is the class property that is needed so we don't create duplicate custom resources with enableRebootForParameterChanges(). I thought it would be clearer to have this property versus making the Provider and CustomResource class properties
There was a problem hiding this comment.
Please add to this docstring that this is only used to guard against repeated invocations of enableRebootForParameterChanges()
| if (!this.rebootForParameterChangesEnabled) { | ||
| this.rebootForParameterChangesEnabled = true; |
There was a problem hiding this comment.
The check on line 607 is all we need for this, no?
There was a problem hiding this comment.
I believe we need it to make sure we don't make a duplicate Custom Resource. I could create class variables to make the function idempotent, but thought that may be overkill
There was a problem hiding this comment.
making the function idempotent is definitely overkill, and guarding against these potential bugs (an error would be thrown complaining about duplicate construct IDs) is definitely the right call in general. In this case though I don't see how the function could be called twice, as it's only called in the constructor and it's internal-only.
There was a problem hiding this comment.
I originally thought about making it constructor only, but It's currently not (example). I figured that exposing the method would be friendlier for situationally enabling the feature under specific conditions (ex. beta/prod stages, using the addToParameterGroup method and then using this method).
There was a problem hiding this comment.
hmmm okay. I don't love that we have this prop and this internal member, but it is a better user experience to be able to use a convenience method to set it, so we can leave it.
Please change the format of this to a guard clause though, eg:
if (this.rebootForParameterChangesEnabled) {
return;
}
this.rebootForParameterChangesEnabled = true;
…boot-handler/index.ts Co-authored-by: Calvin Combs <[email protected]>
mergify
bot
dismissed
comcalvi’s stale review
Pull request has been modified.
| if (!this.rebootForParameterChangesEnabled) { | ||
| this.rebootForParameterChangesEnabled = true; |
There was a problem hiding this comment.
making the function idempotent is definitely overkill, and guarding against these potential bugs (an error would be thrown complaining about duplicate construct IDs) is definitely the right call in general. In this case though I don't see how the function could be called twice, as it's only called in the constructor and it's internal-only.
| if (!this.rebootForParameterChangesEnabled) { | ||
| this.rebootForParameterChangesEnabled = true; |
There was a problem hiding this comment.
hmmm okay. I don't love that we have this prop and this internal member, but it is a better user experience to be able to use a convenience method to set it, so we can leave it.
Please change the format of this to a guard clause though, eg:
if (this.rebootForParameterChangesEnabled) {
return;
}
this.rebootForParameterChangesEnabled = true;
| /** | ||
| * Whether the cluster will be rebooted when changes to the cluster's parameter group that require a restart to apply. | ||
| */ | ||
| protected rebootForParameterChangesEnabled?: boolean; |
There was a problem hiding this comment.
Please add to this docstring that this is only used to guard against repeated invocations of enableRebootForParameterChanges()
mergify
bot
dismissed
comcalvi’s stale review
Pull request has been modified.
|
I got around needing the if (this.node.tryFindChild('RedshiftClusterRebooterCustomResource')) {
return;
} |
|
The |
|
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). |
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). |
Closes #22009
Currently waiting on #22055 and #22059 for the assertions in the integration test to successfully run
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license