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.

Sign up for GitHub

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

Jump to bottom

feat(redshift): optionally reboot Clusters to apply parameter changes #22063

Merged

mergify merged 25 commits into aws:main from dontirun:feat-reboot-cluster-for-parameter-updates

Feb 17, 2023

Contributor

dontirun commented Sep 15, 2022

Closes #22009

Currently waiting on #22055 and #22059 for the assertions in the integration test to successfully run

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

dontirun and others added 3 commits

September 12, 2022 22:44


          inital implementation

8b8798c


          add lambda handler

68ebe28


          documentation, tests, and bugfixes

f262a52

gitpod-io bot commented Sep 15, 2022

aws-cdk-automation requested a review from a team

September 15, 2022 19:52

github-actions bot added effort/medium feature-request p2 labels

dontirun commented

View reviewed changes

packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts

+                      await redshift.rebootCluster({ ClusterIdentifier: clusterId }).promise();
+                    } catch (err) {
+                      if ((<any>err).code === 'InvalidClusterState') {
+                        return await executeActionForStatus(status, 30000);

Contributor Author

dontirun Sep 15, 2022

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 ~~feat(redshift-alpha): optionally reboot Clusters to apply parameter changes~~ feat(redshift): optionally reboot Clusters to apply parameter changes

Arun Donti added 2 commits

September 30, 2022 10:55


          Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

6c1122b


          Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

c61f994

Collaborator

aws-cdk-automation commented Oct 7, 2022

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.

Contributor Author

dontirun commented Oct 7, 2022

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.

Arun Donti added 7 commits

October 10, 2022 12:13


          Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

6fe9387


          fix: allow for enabling the reboot feature before the parameter group…

63400f6

… is creted


          test: comment out assertions

a5d1ee5


          chore: commit deleted asstes

153345a


          chore: update snapshot

f31b6dd


          Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

a01c334


          tests: regenerate snapshot

52916e6

dontirun force-pushed the feat-reboot-cluster-for-parameter-updates branch from 157b2ac to 52916e6 Compare

October 19, 2022 16:41

Collaborator

aws-cdk-automation commented Nov 10, 2022

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.

Arun Donti added 3 commits

November 16, 2022 11:52


          chore: commenting out diff assets in integ test

c445b0f


          docs: add testing procedure to integ test

26fbbaa


          Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

b732dcf

Contributor

mrgrain commented Dec 8, 2022

@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 marked this pull request as draft

December 8, 2022 13:20

Contributor Author

dontirun commented Dec 8, 2022

@mrgrain I sadly couldn't get a few of things working with integ-tests library.

The commented tests didn't work due to limitations with custom resource/lambda response size limitations (can't filter the response)
Diff assets should be enabled, but I kept getting different asset hashes in the codebuild report although it would verify locally

Contributor

mrgrain commented Dec 8, 2022

@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 commented tests didn't work due to limitations with custom resource/lambda response size limitations (can't filter the response)

The PR you've linked as been merged. Is something else missing?

Diff assets should be enabled, but I kept getting different asset hashes in the codebuild report although it would verify locally

Why does it need to be enabled? But yes, it's weird you get a different hash.

Collaborator

aws-cdk-automation commented Feb 2, 2023

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

Collaborator

aws-cdk-automation commented Feb 3, 2023

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.

Collaborator

aws-cdk-automation commented Feb 4, 2023

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.

Collaborator

aws-cdk-automation commented Feb 5, 2023

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.

Collaborator

aws-cdk-automation commented Feb 6, 2023

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.

Collaborator

aws-cdk-automation commented Feb 7, 2023

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.

Collaborator

aws-cdk-automation commented Feb 8, 2023

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.

Collaborator

aws-cdk-automation commented Feb 9, 2023

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.

comcalvi previously requested changes

View reviewed changes

Contributor

comcalvi left a comment

Very interesting feature! There's a few things I don't quite understand yet

packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-redshift/lib/cluster.ts Outdated

+                        ParameterGroupName: Lazy.string({
+                          produce: () => {
+                            if (!this.parameterGroup) {
+                              throw new Error('Cannot enable reboot for parameter changes when there is no associated ClusterParameterGroup.');

Contributor

comcalvi Feb 9, 2023

why?

Contributor Author

dontirun Feb 14, 2023

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)


          refactor: removing unknown-error from reboot actions

b6c323d

mergify bot dismissed comcalvi’s stale review

February 14, 2023 15:55

Pull request has been modified.


          Merge branch 'master' into feat-reboot-cluster-for-parameter-updates

1c69185

dontirun requested a review from comcalvi

February 14, 2023 16:35

comcalvi previously requested changes

View reviewed changes

packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-reboot-handler/index.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-redshift/lib/cluster.ts Outdated

+                /**
+                 * Whether the cluster will be rebooted when changes to the cluster's parameter group that require a restart to apply.
+                 */
+                protected rebootForParameterChangesEnabled?: boolean;

Contributor

comcalvi Feb 14, 2023

I don't think we need this prop, can we remove it? It seems redundant with rebootForParameterChanges?

Contributor Author

dontirun Feb 14, 2023

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

Contributor

comcalvi Feb 16, 2023

Please add to this docstring that this is only used to guard against repeated invocations of enableRebootForParameterChanges()

packages/@aws-cdk/aws-redshift/lib/cluster.ts Outdated

Comment on lines 710 to 711

		if (!this.rebootForParameterChangesEnabled) {
		this.rebootForParameterChangesEnabled = true;

Contributor

comcalvi Feb 14, 2023

The check on line 607 is all we need for this, no?

Contributor Author

dontirun Feb 14, 2023

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

Contributor

comcalvi Feb 15, 2023

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.

Contributor Author

dontirun Feb 15, 2023

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

Contributor

comcalvi Feb 16, 2023

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;


          Update packages/@aws-cdk/aws-redshift/lib/cluster-parameter-change-re…

80848a9

…boot-handler/index.ts

Co-authored-by: Calvin Combs <[email protected]>

mergify bot dismissed comcalvi’s stale review

February 14, 2023 20:50

Pull request has been modified.

comcalvi requested changes

View reviewed changes

packages/@aws-cdk/aws-redshift/lib/cluster.ts Outdated

Comment on lines 710 to 711

		if (!this.rebootForParameterChangesEnabled) {
		this.rebootForParameterChangesEnabled = true;

Contributor

comcalvi Feb 15, 2023

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.

comcalvi previously requested changes

View reviewed changes

packages/@aws-cdk/aws-redshift/lib/cluster.ts Outdated

Comment on lines 710 to 711

		if (!this.rebootForParameterChangesEnabled) {
		this.rebootForParameterChangesEnabled = true;

Contributor

comcalvi Feb 16, 2023

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;

packages/@aws-cdk/aws-redshift/lib/cluster.ts Outdated

+                /**
+                 * Whether the cluster will be rebooted when changes to the cluster's parameter group that require a restart to apply.
+                 */
+                protected rebootForParameterChangesEnabled?: boolean;

Contributor

comcalvi Feb 16, 2023

Please add to this docstring that this is only used to guard against repeated invocations of enableRebootForParameterChanges()


          refactor: use guard clause for clarity

f91ffeb

mergify bot dismissed comcalvi’s stale review

February 16, 2023 21:54

Pull request has been modified.


          refactor: update guard clause

a76b887

Contributor Author

dontirun commented Feb 17, 2023 •

edited

Loading

I got around needing the rebootForParameterChangesEnabled internel member by using node.tryFindChild

    if (this.node.tryFindChild('RedshiftClusterRebooterCustomResource')) {
      return;
    }

comcalvi approved these changes

View reviewed changes

Contributor

comcalvi commented Feb 17, 2023

The node.tryFindChild() solution is perfect! Nice work

Contributor

mergify bot commented Feb 17, 2023

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


          Merge branch 'main' into feat-reboot-cluster-for-parameter-updates

16ba85f

Collaborator

aws-cdk-automation commented Feb 17, 2023

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 16ba85f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify bot merged commit f61d950 into aws:main

Contributor

mergify bot commented Feb 17, 2023

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

filletofish mentioned this pull request

feat(scheduler): ScheduleGroup #26196

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/medium feature-request p2