feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

skinny85 · 2020-07-09T02:01:11Z

The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes #8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
use ParameterGroup instead

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

packages/@aws-cdk/aws-rds/lib/cluster.ts

njlynch · 2020-07-09T08:43:37Z

packages/@aws-cdk/aws-rds/lib/parameter-group.ts

   */
-  protected parameters?: { [key: string]: string } = {};
+  bindToCluster(options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig;


General question: What's the advantage of requiring an empty interface as a parameter to the call? You wouldn't be able to add a new required member to that interface without it being a breaking change. I get it's future-proofing, but doesn't seem to add any more backwards compatibility than just omitting the options for now and making an optional options parameter in the future when needed. Am I missing a non-obvious advantage to this approach?

It's mainly for other languages that we support that don't have optional parameters (Java comes to mind).

I don't think making the options optional in the bindToCluster() method is a good idea - makes it very inconvenient to actually implement it (you have to constantly check for null, which is even worse in languages like Java).

ACK on the null checks. You're still going to have to either (a) make a breaking change or (b) introduce an optional member to the options (which will need a null check). I guess it's the over/under on that degree of awkwardness vs the awkwardness of creating an empty object that (currently) serves no purpose. Again, I'm thinking of the poor Java dev who has to write new ParameterGroupClusterBindOptions().

However, I don't have super-strong feelings either way, just wanting to understand your perspective on the general principle.

ACK on the null checks. You're still going to have to either (a) make a breaking change or (b) introduce an optional member to the options (which will need a null check).

Right. But making it optional would require 2 null checks! Which is twice as irritating 🙂 (Even with an optional parameter, you can't add a required property to ParameterGroupClusterBindOptions without breaking backwards compatibility).

I guess it's the over/under on that degree of awkwardness vs the awkwardness of creating an empty object that (currently) serves no purpose. Again, I'm thinking of the poor Java dev who has to write new ParameterGroupClusterBindOptions().

But here's the beauty of this - a Java dev will never have to create a ParameterGroupClusterBindOptions! Notice that bindToCluster and bindToInstance are only called in Cluster and Instance. So while you might conceivably want to implement bindToCluster() / bindToInstance(), you won't have to call it unless you're doing something really, really crazy (at which point creating an empty ParameterGroupClusterBindOptions is the least of your problems 😉).

Does this make more sense now in how is this approached here?

njlynch · 2020-07-09T09:02:36Z

packages/@aws-cdk/aws-rds/lib/parameter-group.ts

+
+  public bindToCluster(_options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig {
+    if (!this.clusterCfnGroup) {
+      const parentScope = this.instanceCfnGroup ?? this;


This maybe deserves a comment. Why parent the scope of one param group onto the other?

It's because I want both of the Cfn* resources to have the ID 'Resource', but if one of the bind*() methods was already called, that would result in a duplicate identifier in the this scope. Hence this trick.

¯_(ツ)_/¯ I'd add a comment to that effect, unless this is a super-common pattern I haven't seen before.

I was also thinking - instead of changing the parent, I could change the ID.

So, this would become:

public bindToCluster(_options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig { if (!this.clusterCfnGroup) { const resourceId = this.instanceCfnGroup ? 'SecondResource' : 'Resource'; this.clusterCfnGroup = new CfnDBClusterParameterGroup(this, resourceId, { description: this.description || `Cluster parameter group for ${this.family}`, family: this.family, parameters: Lazy.anyValue({ produce: () => this.parameters }), });

Is that more clear @njlynch ?

Any reason not to just do this?

public bindToCluster(...) { this.clusterCfnGroup = new CfnDBClusterParameterGroup(this, 'ClusterParamGroup', { ... }); } public bindToInstance(...) { this.clusterCfnGroup = new CfnDBClusterParameterGroup(this, 'InstanceParamGroup', { ... }); }

i find them both equally readable, but a comment will be handy for posterity as I think it won't be intuitive to a new contributor why we do this

Any reason not to just do this?

public bindToCluster(...) { this.clusterCfnGroup = new CfnDBClusterParameterGroup(this, 'ClusterParamGroup', { ... }); } public bindToInstance(...) { this.clusterCfnGroup = new CfnDBClusterParameterGroup(this, 'InstanceParamGroup', { ... }); }

Actually, yes.

Since the existing classes use the Resource IDs, changing them now to 'ClusterParamGroup'/'InstanceParamGroup' would cause a replacement of them in customer's CloudFormation Stacks after they upgrade their version of CDK, which is quite a disruptive change.

Also, the name 'Resource' (and 'Default') is treated a little bit especially - it's the return value from the this.node.defaultChild method (which is often used with escape hatches).

skinny85 · 2020-07-09T22:55:20Z

Rebased on top of #8686 , which was merged.

packages/@aws-cdk/aws-rds/lib/cluster.ts

njlynch · 2020-07-10T08:15:55Z

packages/@aws-cdk/aws-rds/lib/parameter-group.ts

   */
-  protected parameters?: { [key: string]: string } = {};
+  bindToCluster(options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig;


ACK on the null checks. You're still going to have to either (a) make a breaking change or (b) introduce an optional member to the options (which will need a null check). I guess it's the over/under on that degree of awkwardness vs the awkwardness of creating an empty object that (currently) serves no purpose. Again, I'm thinking of the poor Java dev who has to write new ParameterGroupClusterBindOptions().

However, I don't have super-strong feelings either way, just wanting to understand your perspective on the general principle.

njlynch · 2020-07-10T08:18:14Z

packages/@aws-cdk/aws-rds/lib/parameter-group.ts

+
+  public bindToCluster(_options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig {
+    if (!this.clusterCfnGroup) {
+      const parentScope = this.instanceCfnGroup ?? this;


¯_(ツ)_/¯ I'd add a comment to that effect, unless this is a super-common pattern I haven't seen before.

shivlaks

good simplification! added some minor comments

shivlaks · 2020-07-10T16:02:49Z

packages/@aws-cdk/aws-rds/lib/parameter-group.ts

+   * @param key The key of the parameter to be added
+   * @param value The value of the parameter to be added
+   */
+  public addParameter(key: string, value: string): boolean {


what's the use case where the return type would be used?
at a cursory glance, most of the addXyZ methods return void or a non-primitive type

Sure, but if you're going to return void, you might as well return boolean (I don't see any downsides in making it return boolean compared to void, seems like pure upside to me).

packages/@aws-cdk/aws-rds/test/test.parameter-group.ts

shivlaks · 2020-07-10T16:07:44Z

packages/@aws-cdk/aws-rds/lib/parameter-group.ts

+
+  public bindToCluster(_options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig {
+    if (!this.clusterCfnGroup) {
+      const parentScope = this.instanceCfnGroup ?? this;


i find them both equally readable, but a comment will be handy for posterity as I think it won't be intuitive to a new contributor why we do this

skinny85 · 2020-07-11T01:58:52Z

@njlynch @shivlaks thanks for the review! I wanted to run one more thing by you.

As you see, the most important parameter when creating the ParameterGroup is the family. In general, that is a property of the database engine (either cluster, or instance): parameterGroupFamily. I'm wondering whether it makes more sense for ParameterGroup to take an IEngine in its constructor, if that's the case, similarly to how OptionGroup takes an InstanceEngine today:

aws-cdk/packages/@aws-cdk/aws-rds/lib/option-group.ts

Line 65 in 403d657

readonly engine: IInstanceEngine;

Thoughts on this idea?

njlynch · 2020-07-13T09:24:58Z

In general, that is a property of the database engine (either cluster, or instance): parameterGroupFamily. I'm wondering whether it makes more sense for ParameterGroup to take an IInstanceEngine in its constructor

I'm not familiar enough with the RDS API to see how this works. IInstanceEngine doesn't expose the family today, and the various implementations have a mapping of version to families, rather than just a single family, where version is optional. If you accepted an IInstanceEngine instead of the family, you'd need the various engines to be able to select a "default" family in the case where no version was specified. Is that correct?

skinny85 · 2020-07-13T16:21:08Z

In general, that is a property of the database engine (either cluster, or instance): parameterGroupFamily. I'm wondering whether it makes more sense for ParameterGroup to take an IInstanceEngine in its constructor

I'm not familiar enough with the RDS API to see how this works. IInstanceEngine doesn't expose the family today

Yes, we of course, we would surface parameterGroupFamily in IInstanceEngine for this (actually, thinking about it, we should do it even in this current form of this PR! Because otherwise, it's difficult to know what to pass for family when creating a ParameterGroup even today, and with this, you could use something like DatabaseInstanceEngine.MARIADB.parameterGroupFamily, which makes a lot of sense).

, and the various implementations have a mapping of version to families, rather than just a single family, where version is optional. If you accepted an IInstanceEngine instead of the family, you'd need the various engines to be able to select a "default" family in the case where no version was specified. Is that correct?

Yes, but this the situation that we have today already. For example, if you don't specify a version for the Aurora PostgreSQL Engine, it will default to the parameter group family aurora-postgresql11.

skinny85 · 2020-07-14T02:09:00Z

I've made the change to make ParameterGroup take an IEngine in its construction properties instead of just a parameterGroupFamily. @njlynch @shivlaks would appreciate a look - let me know how you like it!

shivlaks

I like the simplification to IEngine
had a couple small comments / questions around them

packages/@aws-cdk/aws-rds/lib/engine.ts

njlynch · 2020-07-16T16:00:13Z

packages/@aws-cdk/aws-rds/lib/private/parameter-group-family-mapping.ts

+ * When `engineVersion` is specified, return the parameter group family corresponding to that engine version.
+ * Return undefined if no parameter group family is defined for this engine or for the requested `engineVersion`.
+ */
+export function calculateParameterGroupFamily(


I'm having a hard time reconciling this with #9016. In the other PR, parameterGroupFamilies look to be completely removed; will this function be deleted then?

Yep. In that PR, the major version is set explicitly for a specific version (instead of being calculated).

njlynch · 2020-07-16T16:01:28Z

packages/@aws-cdk/aws-rds/lib/private/parameter-group-family-mapping.ts

+ * Return undefined if no parameter group family is defined for this engine or for the requested `engineVersion`.
+ */
+export function calculateParameterGroupFamily(
+  parameterGroupFamilies: ParameterGroupFamilyMapping[] | undefined,


(For my own edification): Is there any logical difference between a required parameter that can be undefined vs an optional parameter, especially in cases where all parameters are the same (e.g., no issues with optional parameters needing to be the last ones)?

There is a tiny difference: for a required parameter that can be undefined, you have to always explicitly pass it (even if just the undefined value), while an optional parameter can be skipped from the call altogether.

njlynch · 2020-07-16T16:02:17Z

packages/@aws-cdk/aws-rds/lib/private/parameter-group-family-mapping.ts

+    return undefined;
+  }
+  if (engineVersion !== undefined) {
+    const family = parameterGroupFamilies.find(x => engineVersion.startsWith(x.engineMajorVersion));


Is this guaranteed to only match one version? If not, is ordering guaranteed? Below, you sort the families by version, which implies this list isn't (guaranteed) sorted.

I think it's guaranteed to match one version, provided the engineMajorVersion are unique, and not subsets of each other, but this is the case with the currently used major versions (and it makes sense, because it would be weird if 11 and 11.2 were both major versions ;p).

The classes ParameterGroup and ClusterParameterGroup were basically identical; they only differed in the L1 that was created in their scope. Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance, which allows us to only have a single class instead of 2. Fixes aws#8932 BREAKING CHANGE: the class ClusterParameterGroup has been removed - use ParameterGroup instead

mergify · 2020-07-16T17:49:57Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-07-16T18:31:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 545c5cd
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-07-16T18:31:47Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

The classes ParameterGroup and ClusterParameterGroup were basically identical; they only differed in the L1 that was created in their scope. Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance, which allows us to only have a single class instead of 2. Fixes aws#8932 BREAKING CHANGE: the class ClusterParameterGroup has been removed - use ParameterGroup instead ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

skinny85 requested review from njlynch and shivlaks July 9, 2020 02:01

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 9, 2020

skinny85 self-assigned this Jul 9, 2020

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Jul 9, 2020

njlynch reviewed Jul 9, 2020

View reviewed changes

skinny85 requested a review from njlynch July 9, 2020 19:10

skinny85 force-pushed the feat/rds-paramaeter-group branch from 40c3e40 to b5f9906 Compare July 9, 2020 22:54

skinny85 force-pushed the feat/rds-paramaeter-group branch from b5f9906 to 2cb2fa5 Compare July 9, 2020 23:02

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Jul 10, 2020

njlynch approved these changes Jul 10, 2020

View reviewed changes

shivlaks approved these changes Jul 10, 2020

View reviewed changes

skinny85 force-pushed the feat/rds-paramaeter-group branch 2 times, most recently from cac12ed to 64b1ba2 Compare July 14, 2020 02:07

skinny85 requested review from njlynch and shivlaks July 14, 2020 02:10

shivlaks approved these changes Jul 15, 2020

View reviewed changes

packages/@aws-cdk/aws-rds/lib/engine.ts Show resolved Hide resolved

packages/@aws-cdk/aws-rds/lib/engine.ts Outdated Show resolved Hide resolved

njlynch reviewed Jul 16, 2020

View reviewed changes

skinny85 added 3 commits July 16, 2020 10:22

Add parameterGroupFamily to IInstanceEngine.

a347bb4

Changed ParameterGroupProps.family to engine.

a64ba83

skinny85 force-pushed the feat/rds-paramaeter-group branch from 64b1ba2 to a64ba83 Compare July 16, 2020 17:30

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Jul 16, 2020

Merge branch 'master' into feat/rds-paramaeter-group

545c5cd

mergify bot merged commit 17b690b into aws:master Jul 16, 2020

skinny85 deleted the feat/rds-paramaeter-group branch July 16, 2020 18:33

mullermp mentioned this pull request Jul 27, 2020

ClusterParameterGroup no longer exists awslabs/aws-rails-provisioner#14

Closed

Chriscbr mentioned this pull request Aug 15, 2020

[aws-rds] ParameterGroup is missing from CloudFormation template #9741

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

skinny85 commented Jul 9, 2020

njlynch Jul 9, 2020

skinny85 Jul 9, 2020

njlynch Jul 10, 2020

skinny85 Jul 10, 2020

njlynch Jul 9, 2020

skinny85 Jul 9, 2020

njlynch Jul 10, 2020

skinny85 Jul 10, 2020

njlynch Jul 10, 2020

shivlaks Jul 10, 2020

skinny85 Jul 10, 2020

skinny85 commented Jul 9, 2020

njlynch Jul 10, 2020

njlynch Jul 10, 2020

shivlaks left a comment

shivlaks Jul 10, 2020

skinny85 Jul 14, 2020

shivlaks Jul 10, 2020

skinny85 commented Jul 11, 2020

njlynch commented Jul 13, 2020 •

edited

Loading

skinny85 commented Jul 13, 2020 •

edited

Loading

skinny85 commented Jul 14, 2020

shivlaks left a comment

njlynch Jul 16, 2020

skinny85 Jul 16, 2020

njlynch Jul 16, 2020

skinny85 Jul 16, 2020

njlynch Jul 16, 2020

skinny85 Jul 16, 2020

mergify bot commented Jul 16, 2020

aws-cdk-automation commented Jul 16, 2020

mergify bot commented Jul 16, 2020

feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

Conversation

skinny85 commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Jul 11, 2020

njlynch commented Jul 13, 2020 • edited Loading

skinny85 commented Jul 13, 2020 • edited Loading

skinny85 commented Jul 14, 2020

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jul 16, 2020

aws-cdk-automation commented Jul 16, 2020

AWS CodeBuild CI Report

mergify bot commented Jul 16, 2020

njlynch commented Jul 13, 2020 •

edited

Loading

skinny85 commented Jul 13, 2020 •

edited

Loading