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

feat(glue): Job construct #12506

Merged
merged 67 commits into from
Sep 8, 2021
Merged

feat(glue): Job construct #12506

merged 67 commits into from
Sep 8, 2021

Conversation

humanzz
Copy link
Contributor

@humanzz humanzz commented Jan 14, 2021

Closes #12443


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

@gitpod-io
Copy link

gitpod-io bot commented Jan 14, 2021

@github-actions github-actions bot added the @aws-cdk/aws-glue Related to AWS Glue label Jan 14, 2021
@humanzz
Copy link
Contributor Author

humanzz commented Jan 14, 2021

This is still a WIP since it depends on #12444 and #12450 being merged first. It can be reviewed for comments and below is a summary of what's introduced in this change

@humanzz humanzz mentioned this pull request Jan 15, 2021
2 tasks
@iliapolo iliapolo changed the title feat(aws-glue): add Job construct (#12443) feat(glue): add Job construct (#12443) Jan 15, 2021
@humanzz humanzz changed the title feat(glue): add Job construct (#12443) feat(glue): Job construct (#12443) Feb 15, 2021
@humanzz humanzz changed the title feat(glue): Job construct (#12443) feat(glue): Job construct Feb 15, 2021
@iliapolo iliapolo added p1 effort/medium Medium work item – several days of effort labels Mar 20, 2021
@humanzz
Copy link
Contributor Author

humanzz commented May 27, 2021

Hi @iliapolo, any hopes of actioning this any time soon?

packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Outdated Show resolved Hide resolved
@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 27, 2021
closes #12443

- Add GlueVersion, WorkerType, JobCommandName enum-like classes for future-proofing these values
- Add PythonVersion as a normal enum as it's less likely to change
- Add Job construct
  - CloudFormation base resource is a complex resource with almost all properties being optional and allowed in specific combinations based on command, glue version and worker types
  - It supports multiple job types e.g. ETL, Streaming and Python Shell and languages e.g. Scala and Python. This requires different combinations of props
  - One possibility is to try to create different constructs for the different types but I've opted to do just one base on resembling CloudFormation which does not prevent us from introducing more specialized types later
  - Documentation is not accurate, based on integ test deployments which led me to not having any assertions on valid props combinations
    - https://docs.aws.amazon.com/glue/latest/dg/add-job.html
    - https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-jobs-job.html
  - Not exposing some CloudFormation props
    - allocatedCapacity as it's deprecated in favour of maxCapacity
    - logUri which is not used and is reserved for future use
- glue job does not emit success or failure metrics to cloudwatch metrics. Instead, it emits events to cloudwatch events
- add JobEventState enum for known job states in https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/EventTypes.html#glue-event-types
- add utility methods to create event rules and cloudwatch metrics based on those roles supporting the most common use cases; success, failure and timeout
@humanzz
Copy link
Contributor Author

humanzz commented Jul 12, 2021

Apologies for not noticing the review earlier. I am OOTO at the moment but will attempt to progress this soon.

@BenChaimberg BenChaimberg removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 12, 2021
- introduce JobBase to contain common logic between Job and Import
  - JobBase now handles CloudWatch Event Rules and Rule-based metrics methods
- address the rename comments
- use the private constructor + static of() pattern where relevant
- Make JobProps.glueVersion required
@gitpod-io
Copy link

gitpod-io bot commented Jul 27, 2021

@mergify mergify bot dismissed iliapolo’s stale review July 27, 2021 11:54

Pull request has been modified.

@humanzz
Copy link
Contributor Author

humanzz commented Jul 27, 2021

I've addressed the majority of the comments, @iliapolo
There still remains a couple which I've left comments above about

  • JobCommand and expanding its responsibility to take on the defaultArguments
  • Some @defaults which I'm not really sure about
  • anything else I might have missed :)

@humanzz
Copy link
Contributor Author

humanzz commented Sep 7, 2021

Hi @BenChaimberg, I feel like we're getting close, one final push (of effort) to get this PR over the line :)

Copy link
Contributor

@BenChaimberg BenChaimberg left a comment

Choose a reason for hiding this comment

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

love these tests. I think we're at the finish line now

packages/@aws-cdk/aws-glue/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-glue/test/code.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/test/code.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job.ts Show resolved Hide resolved
packages/@aws-cdk/aws-glue/test/job.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/test/integ.job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/test/integ.job.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/test/integ.job.ts Show resolved Hide resolved
- update README
- use const instead of let and beforeEach where relevant
- assert on stack assets metadata
- add test case of 2 jobs reusing asset
- other minor things
@mergify mergify bot dismissed BenChaimberg’s stale review September 8, 2021 11:28

Pull request has been modified.

- fix bug with code bucket permissions
  - Glue role did not have permissions to read from the code bucket which manifested as DeniedAccess exceptions when attempting to run jobs
  - Update Code.bind to take a Job instead of Construct so it can grant it the necessary read permissions
- Python shell failed saying Glue 2.0 is not supported
  - Console UI for adding a new Python shell jobs shows Glue 1.0 as the only supported version
  - Modified tests to reject using Glue 0.9 but didn't exclude Glue 2.0 or 3.0 for any future support by the new versions
@humanzz
Copy link
Contributor Author

humanzz commented Sep 8, 2021

And now, ready for another review, @BenChaimberg

packages/@aws-cdk/aws-glue/lib/code.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/code.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-glue/lib/job-executable.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed BenChaimberg’s stale review September 8, 2021 18:54

Pull request has been modified.

Copy link
Contributor

@BenChaimberg BenChaimberg left a comment

Choose a reason for hiding this comment

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

one outstanding comment above

@mergify mergify bot dismissed BenChaimberg’s stale review September 8, 2021 22:03

Pull request has been modified.

@humanzz
Copy link
Contributor Author

humanzz commented Sep 8, 2021

one outstanding comment above

🏁 ? 😄

Copy link
Contributor

@BenChaimberg BenChaimberg left a comment

Choose a reason for hiding this comment

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

YAY! A fabulous job and thank you for putting up with my high standards!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 6bb18eb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit fc74110 into aws:master Sep 8, 2021
@mergify
Copy link
Contributor

mergify bot commented Sep 8, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-glue Related to AWS Glue effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-glue): Job construct
4 participants