Skip to content
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

RFC for Glue L2 Construct #498

Merged
merged 18 commits into from
Aug 29, 2023

Conversation

natalie-white-aws
Copy link
Contributor

@natalie-white-aws natalie-white-aws commented Apr 19, 2023

This is a request for comments about the Glue L2 Construct. See #497 for
additional details.

APIs are signed off by @TheRealAmazonKendra.


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

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start! Please see my comments inline.

text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review May 23, 2023 15:32

Pull request has been modified.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still missing the full set of proposed props in some places. Please fill those in so I can get an overall view of the user experience for each construct. This is looking very close to where I can provide sign-off but I'm going to also ask for a second set of reviews from a team mate.

text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review June 6, 2023 15:34

Pull request has been modified.

text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aggregating some important comments that when addressed will make this RFC very strong:

  • move context out of README section and treat the README as an actual artifact for what you would like the README to look like when the feature is released. It should follow similar flow/story to our other READMEs, which could mean some research to get the right tone.
  • because we are building on an existing Glue L2, we are in a unique situation where BREAKING CHANGES need to be called out and scrutinized in their own section.

text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
@natalie-white-aws
Copy link
Contributor Author

Aggregating some important comments that when addressed will make this RFC very strong:

  • move context out of README section and treat the README as an actual artifact for what you would like the README to look like when the feature is released. It should follow similar flow/story to our other READMEs, which could mean some research to get the right tone.
  • because we are building on an existing Glue L2, we are in a unique situation where BREAKING CHANGES need to be called out and scrutinized in their own section.

I'm not following your guidance on the README section - the format of prior RFCs doesn't seem to have a trend or consistency. Would it be better if we removed the README label altogether and just had "Working Backwards" or "Glue Overview"?

…ntax highlighting, updated first section title.
@mergify mergify bot dismissed kaizencc’s stale review June 23, 2023 20:58

Pull request has been modified.

text/0497-glue-l2-construct.md Outdated Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
text/0497-glue-l2-construct.md Show resolved Hide resolved
…added note about why IAM Role is required from the developer rather than being auto-generated by the L2
Comment on lines 157 to 165
numberOrWorkers?: int;

/**
* Worker Type (optional)
* Type of Worker for Glue to use during Job execution
* Enum options: Standard, G.1X, G.2X, G.025X. G.4X, G.8X, Z.2X
* @default G.2X
* */
workerType?: glue.WorkerType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just leaving the comment in this one place, but it stands for each instance here. Since these two are dependent on each other, it should be one field that takes in an enum like class. Something like Worker.G_1X(200) with the input being the number of workers.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worker type and number of workers are independent of each other which is why kept two different field. Customer can set number of workers and keep worker type as default (G.2X)

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 8, 2023 16:03

Pull request has been modified.

@evgenyka evgenyka added the l2-request request for new L2 construct label Aug 10, 2023
…hanged passive verbs to active, added more README style details, added runtime override for Ray jobs
@TheRealAmazonKendra TheRealAmazonKendra added status/final-comment-period Pending final approval status/api-approved API Bar Raiser signed-off the API of this RFC labels Aug 16, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, @natalie-white-aws! I have 2 non-blocking comments so consider this my approval. Not actually clicking approve because I was more focused on the readme section and @TheRealAmazonKendra may have more comments on implementation.

Edit: sorry I see Kendra added api-approved so likely there are no more comments on implementation :)

libraries and tool sets (e.g. deprecated versions of Python).

Optional and required parameters for each job will be enforced via interface
A Job encapsulates a script that connects to data sources, processes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! When you go and write the actual readme in aws-cdk-lib, the first paragraph is the perfect intro + an example. The second paragraph (starting with "This iteraton of the L2 construct") is less readme-ey. We'll call out the breaking changes in the PR description so they show up in the changelog, and customers better know that L2 === opinionated well before reading the readme.

ETL jobs support pySpark and Scala languages, for which there are separate but
similar constructors. ETL jobs default to the G2 worker type, but you can
override this default with other supported worker type values (G1, G2, G4
and G8). ETL jobs defaults to Glue version 4.0, which you can override to 3.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer this sentence to be shown in code actually. Consider removing "which you can override to 3.0" and instead add this to your example:

glue.ScalaSparkEtlJob(this, 'SSEJ', {
  script: glue.Code.fromBucket('bucket', 'path'),
  className: 'com.example.Hi',
  role: iam.IRole,
  workerType: WorkerType.3_0 // default is WorkerType.4_0

Rationale: our examples travel farther than our words. As part of our release process, we search for all constructs to see if they have an inline code example, and if not, we try to source them from the readme. So this example would be a valid option to be the code example for glue.ScalaSparkEtlJob and could show up in the cdk docs for the construct. We don't do any magic for our prose, so that will definitely only be in the readme.

This is just one example, but if you see opportunity to improve examples elsewhere, I encourage you to take it with the knowledge that examples end up having more influence.

@TheRealAmazonKendra TheRealAmazonKendra merged commit c2883c7 into aws:main Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l2-request request for new L2 construct status/api-approved API Bar Raiser signed-off the API of this RFC status/final-comment-period Pending final approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants