feat(aws-kinesisanalyticsv2): L2 construct for Flink applications #12464

mitchlloyd · 2021-01-11T18:09:34Z

Opened for #12407.

Notes for review:

Flink and SQL applications share almost no properties so having separate ka.FlinkApplication and ka.SqlApplication constructs seems correct. I can't see why these would even share an abstract base class.
I'm trying to focus on shipping the Flink construct before SQL since they are so different and I haven't used an SQL application.
I unested lots of the configuration for discoverability. The Cfn naming is verbose (usually with prefixes) so collisions are unlikely. This is a pretty good example of using CDK today to build a Flink app.

Running List of Open Questions

aws-kinesisanalytics exports both aws-kinesisanalytics and aws-kinesisanalyticsv2 generated code. How should we resolve this? Currently I've exported aws-kinesisanalyticsv2 from aws-kinesisanalyticsv2 and haven't changed the other package. Kept this package isolated from aws-kinesisanalytics at the expense of duplicate generated code.
I'm not confident with the use cases for the fromAttributes factory. I'd prefer to leave this factory method out in this initial PR if possible, but I'm also open to comments about what use cases this should handle.
All logging options could be flattened into FlinkApplicationProps (e.g. logRentention, logGroupName, logStreamName, logEncryptionKey...). My first thought was to provide a new ka.Logging type that can be passed to customize the logGroup and logStream. There may be some other middle ground that could let users pass in a logGroup and then a logStream. Went with the flat list of props added to FlinkApplicationProps
~~When I run yarn build I get this error even though I've already tagged the class.~~
Was missing a gen yarn script entry.
Should this module become aws-flink-application or aws-kinesisanalytics-flink to side step the confusion of having seemingly unrelated constructs in the same module at the expense of clashing with CloudFormation? New module name: aws-kinesisanalytics-flink.

Todo

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

gitpod-io · 2021-01-11T18:09:37Z

mergify · 2021-01-11T18:13:07Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

mitchlloyd · 2021-01-15T21:57:36Z

@iliapolo Any feedback on the README portion of this PR or is it time to proceed with implementation?

iliapolo · 2021-01-17T13:25:44Z

@mitchlloyd

@iliapolo Any feedback on the README portion of this PR or is it time to proceed with implementation?

This is great. Let's start with the implementation. I love that you decided to create a concrete construct for Flink rather than creating a generic Application with a bunch of different configuration types 👍

github-actions · 2021-01-25T06:52:17Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

mitchlloyd · 2021-01-25T16:54:44Z

Still working on this. Will update PR description with progress. One important item to consider is how to deal with the fact that the aws-kinesisanalytics module already exports both AWS::KinesisAnalytics and AWS::KinesisAnalyticsV2 generated Cfn code. For now I've exported AWS::KinesisAnalyticsV2 from the aws-kinesisanalyticsv2 module but this means we would have duplicate code in the CDK.

mitchlloyd · 2021-01-28T22:08:51Z

@skinny85 Would it be possible to get some early feedback on this PR? I've added a list of open questions to the PR description where I could use some guidance.

skinny85 · 2021-01-29T00:06:38Z

@skinny85 Would it be possible to get some early feedback on this PR? I've added a list of open questions to the PR description where I could use some guidance.

I'll take a detailed look tomorrow, I have it on my ToDo list 🙂

skinny85 · 2021-01-29T00:07:03Z

In the meantime, any chance of getting rid of the yarn.lock changes? 🙂

Copied example-resource construct.

The trade off here is that we won't allow users to control the stream properties.

mitchlloyd · 2021-02-08T00:07:28Z

aws-lambda-python package is failing with error: Can not find Rust compiler Not sure if this is related to changes in this PR.

…

On Sun, Feb 7, 2021 at 3:57 PM AWS CDK Automation ***@***.***> wrote: AWS CodeBuild CI Report - CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o - Commit ID: ad2f743 <ad2f743> - Result: FAILED - Build Logs <https://xuogv3vm9j.execute-api.us-east-1.amazonaws.com/Prod/buildlogs?key=007d399a-7e9b-4d42-a371-03af476b08c1%2Fbuild.log> (available for 30 days) *Powered by github-codebuild-logs <https://serverlessrepo.aws.amazon.com/applications/arn:aws:serverlessrepo:us-east-1:277187709615:applications~github-codebuild-logs>, available on the AWS Serverless Application Repository <https://aws.amazon.com/serverless/serverlessrepo/>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12464 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADWQPJE7EFV4VJKFMLNTTS54SGZANCNFSM4V56BBIQ> .

skinny85 · 2021-02-08T20:01:10Z

aws-lambda-python package is failing with error: Can not find Rust compiler Not sure if this is related to changes in this PR.

That's an unrelated problem, we're working on it!

skinny85

Thanks so much for the contribution @mitchlloyd !

Pull request has been modified.

mergify · 2021-02-09T23:14:41Z

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 · 2021-02-09T23:14:41Z

AWS CodeBuild CI Report

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

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

…s#12464) Opened for aws#12407. Notes for review: 1. Flink and SQL applications share almost no properties so having separate ka.FlinkApplication and ka.SqlApplication constructs seems correct. I can't see why these would even share an abstract base class. 1. I'm trying to focus on shipping the Flink construct before SQL since they are so different and I haven't used an SQL application. 1. I unested lots of the configuration for discoverability. The Cfn naming is verbose (usually with prefixes) so collisions are unlikely. [This](https://github.com/aws-samples/amazon-kinesis-analytics-streaming-etl/blob/master/cdk/lib/streaming-etl.ts#L100) is a pretty good example of using CDK today to build a Flink app. Running List of Open Questions ------------------------------- 1. ~aws-kinesisanalytics exports both `aws-kinesisanalytics` and `aws-kinesisanalyticsv2` generated code. How should we resolve this? Currently I've exported `aws-kinesisanalyticsv2` from `aws-kinesisanalyticsv2` and haven't changed the other package.~ Kept this package isolated from aws-kinesisanalytics at the expense of duplicate generated code. 2. ~I'm not confident with the use cases for the `fromAttributes` factory. I'd prefer to leave this factory method out in this initial PR if possible, but I'm also open to comments about what use cases this should handle.~ 3. ~All logging options could be flattened into FlinkApplicationProps (e.g. logRentention, logGroupName, logStreamName, logEncryptionKey...). My first thought was to provide a new `ka.Logging` type that can be passed to customize the logGroup and logStream. There may be some other middle ground that could let users pass in a logGroup and then a logStream.~ Went with the flat list of props added to FlinkApplicationProps 4. ~When I run `yarn build` I get this error even though I've already tagged the class.~ Was missing a `gen` yarn script entry. 5. ~Should this module become `aws-flink-application` or `aws-kinesisanalytics-flink` to side step the confusion of having seemingly unrelated constructs in the same module at the expense of clashing with CloudFormation?~ New module name: `aws-kinesisanalytics-flink`. Todo ---- - [x] Inline documentation - [x] Add property groups - [x] checkpointEnabled - [x] minPauseBetweenCheckpoints - [x] logLevel - [x] metricsLevel - [x] autoscalingEnabled - [x] parallelism - [x] parallelismPerKpu - [x] snapshotsEnabled - [x] Figure out fromAttributes approach - [x] Add log stream - [x] Decide on logging options approach - [x] Add metrics access - [x] Validate application name - [x] Integration tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

I PR'd the [original version of the aws-kinesisanalytics-flink constructs](#12464) to CDK. I'm following up to add the missing `metric*` methods according to the [design guidelines](https://github.com/aws/aws-cdk/blob/master/docs/DESIGN_GUIDELINES.md#metrics). [Reference for Flink Application metrics](https://docs.aws.amazon.com/kinesisanalytics/latest/java/metrics-dimensions.html). I have a few running Flink apps and I was able to see that KPUs are also reported for the Flink apps. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) * [x] I don't think conventional metric changes require an update to the README. ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [x] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-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*

…19599) I PR'd the [original version of the aws-kinesisanalytics-flink constructs](aws#12464) to CDK. I'm following up to add the missing `metric*` methods according to the [design guidelines](https://github.com/aws/aws-cdk/blob/master/docs/DESIGN_GUIDELINES.md#metrics). [Reference for Flink Application metrics](https://docs.aws.amazon.com/kinesisanalytics/latest/java/metrics-dimensions.html). I have a few running Flink apps and I was able to see that KPUs are also reported for the Flink apps. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) * [x] I don't think conventional metric changes require an update to the README. ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [x] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-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*

mitchlloyd changed the title ~~Start KAV2 README~~ aws-kinesisanalyticsv2: L2 construct for Flink applications Jan 11, 2021

mitchlloyd changed the title ~~aws-kinesisanalyticsv2: L2 construct for Flink applications~~ (aws-kinesisanalyticsv2): L2 construct for Flink applications Jan 11, 2021

mitchlloyd changed the title ~~(aws-kinesisanalyticsv2): L2 construct for Flink applications~~ feat(aws-kinesisanalyticsv2): L2 construct for Flink applications Jan 12, 2021

iliapolo self-assigned this Jan 17, 2021

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 17, 2021

github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 25, 2021

mitchlloyd force-pushed the kinesis-analytics-l2 branch from a84a0cf to c99efb3 Compare January 25, 2021 16:54

github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jan 26, 2021

iliapolo assigned skinny85 and unassigned skinny85 and iliapolo Jan 26, 2021

Mitch Lloyd added 8 commits January 28, 2021 17:07

Start KAV2 README

332f984

Add new construct scaffolding

fedb2cc

Copied example-resource construct.

Add initial FlinkApplication construct

86c3323

Add property groups

ede24f7

Add checkpointingEnabled

41dfbd0

Add minPauseBetweenCheckpoints

29abb1a

Add metricsLevel and logLevel

54df2a7

Finish application settings

a0bb669

Mitch Lloyd added 4 commits February 7, 2021 14:41

Update README for package focus change

2fd5773

Declare interface before use

49dbe4e

Use factory over constructor for FlinkRuntime

0b828fa

Declare one Import class in module scope

36f9eb9

mitchlloyd force-pushed the kinesis-analytics-l2 branch from 40b7b06 to 5742d0a Compare February 7, 2021 22:41

Pass logGroup rather than all log settings

b951b97

The trade off here is that we won't allow users to control the stream properties.

mitchlloyd force-pushed the kinesis-analytics-l2 branch from 3ccf63d to b951b97 Compare February 7, 2021 23:15

Merge branch 'master' into kinesis-analytics-l2

ad2f743

mitchlloyd requested a review from skinny85 February 8, 2021 00:07

skinny85 added 2 commits February 8, 2021 12:30

Remove dependency from cloudformation-include.

4fc6d79

Update ReadMe.md with the latest log properties

2571ab2

skinny85 previously approved these changes Feb 8, 2021

View reviewed changes

Merge branch 'master' into kinesis-analytics-l2

2225c65

skinny85 previously approved these changes Feb 9, 2021

View reviewed changes

Merge branch 'master' into kinesis-analytics-l2

a91b6b1

skinny85 approved these changes Feb 9, 2021

View reviewed changes

mergify bot merged commit 94279f3 into aws:master Feb 9, 2021

mitchlloyd mentioned this pull request Feb 10, 2021

aws-kinesisanalyticsv2: L2 construct for Flink applications #12407

Closed

2 tasks

mitchlloyd deleted the kinesis-analytics-l2 branch February 10, 2021 22:02

JoeKodsi-Idealworks mentioned this pull request Oct 26, 2021

aws_kinesisanalytics: Configuring a vpc #17159

Closed

2 tasks

mitchlloyd mentioned this pull request Mar 28, 2022

feat(kinesisanalytics-flink): Add metrics to Flink applications #19599

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-kinesisanalyticsv2): L2 construct for Flink applications #12464

feat(aws-kinesisanalyticsv2): L2 construct for Flink applications #12464

mitchlloyd commented Jan 11, 2021 •

edited

Loading

gitpod-io bot commented Jan 11, 2021 •

edited

Loading

mergify bot commented Jan 11, 2021

mitchlloyd commented Jan 15, 2021

iliapolo commented Jan 17, 2021

github-actions bot commented Jan 25, 2021

mitchlloyd commented Jan 25, 2021 •

edited

Loading

mitchlloyd commented Jan 28, 2021 •

edited

Loading

skinny85 commented Jan 29, 2021

skinny85 commented Jan 29, 2021

mitchlloyd commented Feb 8, 2021 via email

skinny85 commented Feb 8, 2021

skinny85 left a comment

mergify bot commented Feb 9, 2021

aws-cdk-automation commented Feb 9, 2021

feat(aws-kinesisanalyticsv2): L2 construct for Flink applications #12464

feat(aws-kinesisanalyticsv2): L2 construct for Flink applications #12464

Conversation

mitchlloyd commented Jan 11, 2021 • edited Loading

Running List of Open Questions

Todo

gitpod-io bot commented Jan 11, 2021 • edited Loading

mergify bot commented Jan 11, 2021

mitchlloyd commented Jan 15, 2021

iliapolo commented Jan 17, 2021

github-actions bot commented Jan 25, 2021

mitchlloyd commented Jan 25, 2021 • edited Loading

mitchlloyd commented Jan 28, 2021 • edited Loading

skinny85 commented Jan 29, 2021

skinny85 commented Jan 29, 2021

mitchlloyd commented Feb 8, 2021 via email

skinny85 commented Feb 8, 2021

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 9, 2021

aws-cdk-automation commented Feb 9, 2021

AWS CodeBuild CI Report

mitchlloyd commented Jan 11, 2021 •

edited

Loading

gitpod-io bot commented Jan 11, 2021 •

edited

Loading

mitchlloyd commented Jan 25, 2021 •

edited

Loading

mitchlloyd commented Jan 28, 2021 •

edited

Loading