chore(rds): change the way Engines are modeled #8686

skinny85 · 2020-06-22T21:07:18Z

Change the types of the engine from DatabaseClusterEngine and its subclass, DatabaseInstanceEngine, to 2 separate interfaces, IClusterEngine and IInstanceEngine. Add properties to each of them, including engineVersion, and thus stop taking engineVersion separately as a property for instances, clusters and OptionGroups. Allow creating engines with the version given as an arbitrary string.

Add a bind()-like protocol to both IClusterEngine and IInstanceEngine, which allows expressing validation and default values logic directly in the engines, instead of hard-coding them in cluster and instance.

Because of those changes, creating a default cluster with Aurora MySQL or Postgres engines now works out of the box, instead of failing at deploy time. We also correctly set the port for Postgres clusters to 5432, instead of leaving it as the default 3306 (which is the MySQL port).

Fixes #2213
Fixes #2512
Fixes #4150
Fixes #5126
Fixes #7072

BREAKING CHANGE: the class DatabaseClusterEngine has been replaced with the interface IClusterEngine in the type of DatabaseClusterProps.engine

rds: the class DatabaseInstanceEngine has been replaced with the interface IInstanceEngine in the type of DatabaseInstanceSourceProps.engine
rds: DatabaseClusterProps.engineVersion has been removed; instead, create an IClusterEngine with a specific version using the static factory methods in DatabaseClusterEngine
rds: DatabaseInstanceSourceProps.engineVersion has been removed; instead, create an IInstanceEngine with a specific version using the static factory methods in DatabaseInstanceEngine
rds: the property majorEngineVersion can no longer be passed when creating an OptionGroup; instead, create an IInstanceEngine with a specific version using the static factory methods in DatabaseInstanceEngine

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

skinny85 · 2020-06-22T21:17:18Z

@nija-at @rix0rrr this is a follow-up from #8412, and is ready for your review.

In the interest of keeping this PR small, it doesn't include the following items agreed to in #8506 (comment) :

engineVersion is still modeled as a string, like it was before (issue Better type database engine versions #6532). Will be done in a separate PR.
The classes of engines like AuroraMySqlClusterEngine, etc. are not public. I was worried about how much of the hierarchy of the Engines we would have to make public to make this work, and decided against it for the time being. We can always make them public later, it will be a backwards-compatible change.
The fate of ClusterParameterGroup and ParameterGroup is still not resolved (this comment, and the thread it belongs to: feat(rds): change the way Engines are modeled #8412 (comment) ). @nija-at would appreciate your input here.

packages/@aws-cdk/aws-rds/README.md

nija-at

Comments so far...

packages/@aws-cdk/aws-rds/README.md

nija-at · 2020-06-24T10:27:45Z

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

+export interface ClusterEngineBindOptions {
+  /**
+   * The role used for S3 importing.
+   *
+   * @default - none
+   */
+  readonly s3ImportRole?: iam.IRole;
+
+  /**
+   * The role used for S3 exporting.
+   *
+   *  @default - none
+   */
+  readonly s3ExportRole?: iam.IRole;


These are only relevant for aurora. They should instead be modeled as properties of the aurora engine.

Few things:

I don't think they're actually relevant only for aurora. Let me experiment with it and verify.

If that's true, then we should add validation for the other engines that fails if those have been provided. So they still should be passed here.

These are properties of the Cluster though. That is, you can have 2 Clusters that have the same engine, but import/export from/to different S3 Buckets, using different Roles. I don't think it makes sense for them to be on Engine.

Confirmed this works for PostgreSQL. Docs: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/AuroraPostgreSQL.Migrating.html#USER_PostgreSQL.S3Import

I don't think they're actually relevant only for aurora. Let me experiment with it and verify.

Confirmed this works for PostgreSQL. Docs: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/AuroraPostgreSQL.Migrating.html#USER_PostgreSQL.S3Import

The heading on that article is 'Importing Amazon S3 Data into an Aurora PostgreSQL DB Cluster'. Did you try this on a non-Aurora PostgreSQL?

Can you confirm that the API will be approximately, as we discussed -

new rds.DatabaseCluster(this, 'Cluster', { engine: new rds.AuroraClusterEngine({ s3ImportRole: ..., s3ExportRole: ..., // ... }), });

If not, what will the new API be when customers want to pass in s3ImportRole and s3ExportRole

I don't think they're actually relevant only for aurora. Let me experiment with it and verify.

Confirmed this works for PostgreSQL. Docs: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/AuroraPostgreSQL.Migrating.html#USER_PostgreSQL.S3Import

The heading on that article is 'Importing Amazon S3 Data into an Aurora PostgreSQL DB Cluster'. Did you try this on a non-Aurora PostgreSQL?

There is no cluster engine that's a non-Aurora PostgreSQL.

Either way, these options (S3 import/export) make sense for Clusters with all 3 currently supported engine types, which was the original question.

engineMode is an example of foobar.

I was hoping that we will be solving the problem where all properties are available on instances/clusters irrespective of whether they are supported or not.

As an example, the current (i.e. before this PR) experience allows customers to specify s3ImportRole for even non-Aurora databases which will result in a validation error.
The best experience would be to present the user only with the options that the selected database engine supports.

Did you ever intend for this to be fixed here, or was this not your intention?

As an example, the current (i.e. before this PR) experience allows customers to specify s3ImportRole for even non-Aurora databases which will result in a validation error.

Can you show me where that is? Because I actually don't think that's true.

I was hoping that we will be solving the problem where all properties are available on instances/clusters irrespective of whether they are supported or not.

The best experience would be to present the user only with the options that the selected database engine supports.

Did you ever intend for this to be fixed here, or was this not your intention?

I thought we talked about this topic when we discussed how this API should look like, and we decided that the amount of axes an engine can vary on (family, version, mode, etc.) is too big to make a separate class for each variant. So, each engine can add runtime validation to make sure the properties of the Cluster/Instance it's being used for are correct (and, in your example, error out if it doesn't support S3 imports and/or exports).

I thought we talked about this topic when we discussed how this API should look like, and we decided that the amount of axes an engine can vary on (family, version, mode, etc.) is too big to make a separate class for each variant. So, each engine can add runtime validation to make sure the properties of the Cluster/Instance it's being used for are correct (and, in your example, error out if it doesn't support S3 imports and/or exports).

As far as I can recall, we concluded that this will be the case for engine version and engine mode where it seemed like the number of properties affected are quite small and may not be worth modeling them separately. But I don't recall that being the conclusion for engine type.

I still think modeling the engine types separately and presenting only the options applicable to that engine type is a better ergonomic experience. Hence this issue is still valid #6713. Being closer to the RDS construct library than I, I'll let you decide whether it is or isn't.

I'm also not expecting this to be addressed in this PR (likely my mistake to have assumed that in the first place).

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

skinny85 · 2020-06-24T19:39:32Z

@nija-at can you address the (Cluster)ParameterGroup questions raised in: #8412 (comment) ?

nija-at

Next round of comments. Have got through the code changes for cluster engine and instance engine.

nija-at · 2020-06-30T13:05:42Z

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

+export interface ClusterEngineBindOptions {
+  /**
+   * The role used for S3 importing.
+   *
+   * @default - none
+   */
+  readonly s3ImportRole?: iam.IRole;
+
+  /**
+   * The role used for S3 exporting.
+   *
+   *  @default - none
+   */
+  readonly s3ExportRole?: iam.IRole;


I was hoping that we will be solving the problem where all properties are available on instances/clusters irrespective of whether they are supported or not.

As an example, the current (i.e. before this PR) experience allows customers to specify s3ImportRole for even non-Aurora databases which will result in a validation error.
The best experience would be to present the user only with the options that the selected database engine supports.

Did you ever intend for this to be fixed here, or was this not your intention?

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

nija-at · 2020-06-30T13:50:14Z

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

+  }
+
+  /** Creates a new Oracle Standard Edition instance engine. */
+  public static oracleStandardEdition(props: OracleSeInstanceEngineProps): IInstanceEngine {


oracleSE and oracleEE maybe? Does JSII do the right thing for other languages like Java here?

Changed to oracleSe and oracleEe.

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

nija-at · 2020-07-06T10:37:51Z

@nija-at can you address the (Cluster)ParameterGroup questions raised in: #8412 (comment) ?

Let's keep this change outside of this PR.
How will this work when a user wants to import an existing parameter group, and bind it with an instance/cluster? Is this use case even make sense?
If we're doing this, can we go further and eliminate the ParameterGroup class itself? Keep parameter group a property of instance and cluster and create a CfnParameterGroup as part of DatabaseCluster and DatabaseInstance?.

skinny85 · 2020-07-07T01:04:41Z

@nija-at can you address the (Cluster)ParameterGroup questions raised in: #8412 (comment) ?

How will this work when a user wants to import an existing parameter group, and bind it with an instance/cluster? Is this use case even make sense?

It will work exactly like it does today (the imported ParameterGroup does not create any L1s, so no problem there. In fact, there's a single ParameterGroup.fromParameterGroupName() method today - there's not a separate ClusterParameterGroup.fromParameterGroupName() and InstanceParameterGroup.fromParameterGroupName()).

If we're doing this, can we go further and eliminate the ParameterGroup class itself? Keep parameter group a property of instance and cluster and create a CfnParameterGroup as part of DatabaseCluster and DatabaseInstance?.

No, because of point nr 2 above (customers need to be able to import existing ParameterGroups, including default ones that are created by RDS. In fact, we need it ourselves - we use that capability in the implementations of IClusterEngine).

skinny85 · 2020-07-07T01:19:15Z

@nija-at I've incorporated all of your comments, please re-review!

nija-at · 2020-07-09T09:13:11Z

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

+export interface ClusterEngineBindOptions {
+  /**
+   * The role used for S3 importing.
+   *
+   * @default - none
+   */
+  readonly s3ImportRole?: iam.IRole;
+
+  /**
+   * The role used for S3 exporting.
+   *
+   *  @default - none
+   */
+  readonly s3ExportRole?: iam.IRole;


I thought we talked about this topic when we discussed how this API should look like, and we decided that the amount of axes an engine can vary on (family, version, mode, etc.) is too big to make a separate class for each variant. So, each engine can add runtime validation to make sure the properties of the Cluster/Instance it's being used for are correct (and, in your example, error out if it doesn't support S3 imports and/or exports).

As far as I can recall, we concluded that this will be the case for engine version and engine mode where it seemed like the number of properties affected are quite small and may not be worth modeling them separately. But I don't recall that being the conclusion for engine type.

I still think modeling the engine types separately and presenting only the options applicable to that engine type is a better ergonomic experience. Hence this issue is still valid #6713. Being closer to the RDS construct library than I, I'll let you decide whether it is or isn't.

I'm also not expecting this to be addressed in this PR (likely my mistake to have assumed that in the first place).

nija-at · 2020-07-09T12:48:55Z

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

+   * Return an optional default ParameterGroup,
+   * possibly an imported one,
+   * if one wasn't provided by the customer explicitly.


This needs a bit of work. It's not clear why it should be imported and how would an implementer know if it was provided by the customer.

Comments on protected methods should be written from the point of view of an implementor of this method. They must know what this method should do. They shouldn't have to see how it's used to reverse engineer it.

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

Change the types of the engine from DatabaseClusterEngine and its subclass, DatabaseInstanceEngine, to 2 separate interfaces, IClusterEngine and IInstanceEngine. Add properties to each of them, including engineVersion, and thus stop taking engineVersion separately as a property for instances, clusters and OptionGroups. Allow changing the version of an existing engine to an arbitrary string. Add a bind()-like protocol to both IClusterEngine and IInstanceEngine, which allows expressing validation and default values logic directly in the engines, instead of hard-coding them in cluster and instance. Because of those changes, creating a default cluster with Aurora MySQL or Postgres engines now works out of the box, instead of failing at deploy time. We also correctly set the port for Postgres clusters to 5432, instead of leaving it as the default 3306 (which is the MySQL port). Fixes aws#2213 Fixes aws#2512 Fixes aws#4150 Fixes aws#5126 Fixes aws#7072 BREAKING CHANGE: the class DatabaseClusterEngine has been replaced with the interface IClusterEngine in the type of DatabaseClusterProps.engine * **rds**: the class DatabaseInstanceEngine has been replaced with the interface IInstanceEngine in the type of DatabaseInstanceSourceProps.engine * **rds**: DatabaseClusterProps.engineVersion has been removed; instead, create an IClusterEngine with a specific version using the static factory methods in DatabaseClusterEngine * **rds**: DatabaseInstanceSourceProps.engineVersion has been removed; instead, create an IInstanceEngine with a specific version using the static factory methods in DatabaseInstanceEngine * **rds**: the property majorEngineVersion can no longer be passed when creating an OptionGroup; instead, create an IInstanceEngine with a specific version using the static factory methods in DatabaseInstanceEngine

mergify · 2020-07-09T21:53:26Z

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-09T22:14:26Z

AWS CodeBuild CI Report

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

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

mergify · 2020-07-09T22:14:54Z

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

skinny85 requested review from rix0rrr and nija-at June 22, 2020 21:07

skinny85 mentioned this pull request Jun 22, 2020

feat(rds): change the way Engines are modeled #8412

Closed

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

skinny85 force-pushed the feat/iengine-in-rds branch from d58f8ef to 36f761f Compare June 22, 2020 21:10

skinny85 mentioned this pull request Jun 22, 2020

fix(rds): instance engine can be (wrongly) specified as engine when defining a cluster #8514

Closed

civilizeddev mentioned this pull request Jun 23, 2020

feat(rds): database proxy #8476

Merged

jogold reviewed Jun 23, 2020

View reviewed changes

packages/@aws-cdk/aws-rds/README.md Outdated Show resolved Hide resolved

skinny85 assigned skinny85 and nija-at and unassigned skinny85 Jun 23, 2020

nija-at assigned skinny85 and unassigned nija-at Jun 24, 2020

nija-at changed the title ~~feat(rds): change the way Engines are modeled~~ chore(rds): change the way Engines are modeled Jun 24, 2020

nija-at suggested changes Jun 24, 2020

View reviewed changes

nija-at self-requested a review June 24, 2020 10:55

skinny85 force-pushed the feat/iengine-in-rds branch 2 times, most recently from 2a63a95 to 893608a Compare June 26, 2020 01:08

skinny85 mentioned this pull request Jun 26, 2020

RDS Engine abstraction design issue #8506

Closed

nija-at suggested changes Jun 30, 2020

View reviewed changes

ericzbeard mentioned this pull request Jul 1, 2020

Cannot create Aurora Serverless cluster using RDS Construct #929

Closed

skinny85 force-pushed the feat/iengine-in-rds branch from 893608a to 7861fae Compare July 7, 2020 01:18

skinny85 requested a review from nija-at July 7, 2020 01:18

skinny85 mentioned this pull request Jul 8, 2020

Finalize the design of ParameterGroup in RDS #8932

Closed

skinny85 added this to the RDS to 'developer preview' milestone Jul 8, 2020

nija-at approved these changes Jul 9, 2020

View reviewed changes

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

skinny85 added 15 commits July 9, 2020 14:14

Change 'Base' from prefix to suffix.

daf7b75

Change 'engineVersion' to 'version'.

3592109

Change name defaultAurora -> aurora.

78b3f35

Fix DatabaseProxy test.

c6adc4c

Remove defaultInstanceType from IClusterEngine and IInstanceEngine.

b26d5c7

Drop 'Default' from AuroraClusterEngine(Props).

d10088b

Changes to bindToCluster() in IClusterEngine.

9602764

Changed the name 'postgres' to 'postgreSql'.

59e97b2

Change 'XyzEdition' to 'Xe' in InstanceEngine.

ec52d19

Get rid of NonSqlServerInstanceEngine.

3fe5654

Change OracleInstanceEngineProps.engineVersion to version.

6295e74

Introduce common superinterfaces in XyzEngineProps.

3a5cfd8

Fix DatabaseProxy test again.

2a06e36

mySql -> mysql, postgreSql -> postgres

d23e930

skinny85 force-pushed the feat/iengine-in-rds branch from 7861fae to d23e930 Compare July 9, 2020 21:29

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

Merge branch 'master' into feat/iengine-in-rds

44721bb

mergify bot merged commit 63cc1b4 into aws:master Jul 9, 2020

skinny85 deleted the feat/iengine-in-rds branch July 9, 2020 22:20

skinny85 mentioned this pull request Jul 9, 2020

feat(rds): unify ParameterGroup and ClusterParameterGroup #8959

Merged

civilizeddev mentioned this pull request Jul 25, 2020

fix(rds): Add engine to IDatabaseInstance, IClusterInstance #9257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(rds): change the way Engines are modeled #8686

chore(rds): change the way Engines are modeled #8686

skinny85 commented Jun 22, 2020

skinny85 commented Jun 22, 2020

nija-at left a comment

nija-at Jun 24, 2020

skinny85 Jun 24, 2020

skinny85 Jun 24, 2020

nija-at Jun 25, 2020

skinny85 Jun 25, 2020

skinny85 Jun 26, 2020

nija-at Jun 30, 2020

skinny85 Jul 6, 2020

skinny85 Jul 6, 2020 •

edited

Loading

nija-at Jul 9, 2020

skinny85 commented Jun 24, 2020

nija-at left a comment

nija-at Jun 30, 2020

nija-at Jun 30, 2020

skinny85 Jul 6, 2020

nija-at commented Jul 6, 2020

skinny85 commented Jul 7, 2020 •

edited

Loading

skinny85 commented Jul 7, 2020

nija-at Jul 9, 2020

nija-at Jul 9, 2020

mergify bot commented Jul 9, 2020

aws-cdk-automation commented Jul 9, 2020

mergify bot commented Jul 9, 2020

chore(rds): change the way Engines are modeled #8686

chore(rds): change the way Engines are modeled #8686

Conversation

skinny85 commented Jun 22, 2020

skinny85 commented Jun 22, 2020

nija-at 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Jun 24, 2020

nija-at 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

nija-at commented Jul 6, 2020

skinny85 commented Jul 7, 2020 • edited Loading

skinny85 commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jul 9, 2020

aws-cdk-automation commented Jul 9, 2020

AWS CodeBuild CI Report

mergify bot commented Jul 9, 2020

skinny85 Jul 6, 2020 •

edited

Loading

skinny85 commented Jul 7, 2020 •

edited

Loading