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(iot): add Action to put record to Kinesis Data stream #18321

Merged
merged 14 commits into from
Jan 19, 2022

Conversation

yamatatsu
Copy link
Contributor

Fixes #17703


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 8, 2022

@github-actions github-actions bot added the @aws-cdk/aws-iot Related to AWS IoT label Jan 8, 2022
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Very high quality contribution as always, @yamatatsu! A few minor comments.

sql: iot.IotSql.fromStringAsVer20160323("SELECT * FROM 'device/+/data'"),
actions: [
new actions.KinesisPutRecordAction(stream, {
partitionKey: '${timestamp()}', // optional property
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's mention what's the default.

* The partition key is usually composed of an expression (for example, ${topic()} or ${timestamp()}).
* For more information @see https://docs.aws.amazon.com/kinesis/latest/APIReference/API_PutRecord.html#API_PutRecord_RequestParameters
*
* @default - None
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate what does "None" mean here?

Copy link
Contributor Author

@yamatatsu yamatatsu Jan 11, 2022

Choose a reason for hiding this comment

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

Sorry, I tried to elaborate, but I could not😞.

Proberbly, IoT Core rule is fill default value (e.g. MQTT payload JSON string or hash of it) because PartitionKey is required in this API Reference.
But there is no description about it in IoT Core rule documentation and CloudFormation Documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try deploying an IoT app without this property filled?

Maybe this is simply a mistake in the CloudFormation resource, and this should actually be required?

Copy link
Contributor Author

@yamatatsu yamatatsu Jan 12, 2022

Choose a reason for hiding this comment

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

Did you try deploying an IoT app without this property filled?

Yes I tried and the deploying secceeded.

Again, I deployed, and I tested following for confirmation to be spreaded records to shards:

  • Prepare
    • Deploy Kinesis Stream with 10 shards
  • Test1
    • Prepare: Deploy IoT Core Kinesis Rule action with ${timestamp()} partitionKey
    • Send 10 same payloads
  • Test2
    • Prepare: Deploy IoT Core Kinesis Rule action with no partitionKey
    • Send 10 same payloads
  • Test3
    • Prepare: Deploy IoT Core Kinesis Rule action with no partitionKey
    • Send 10 payloads different each time

Results:

  • Test1: Records was spreaded to each shards
  • Test2: All records was put in same record
  • Test3: All records was put in same record

The IoT Core may be filling in a static partitionKey if it is not specified.
But this is just a guess...

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think that's a good guess. Let's put that in the docs - it's certainly better than just "None", which gives you no clue what that actually means 🙂.

Thanks for the detailed testing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

OK. If we beleave this guess, I think it is better that fill default value. Because in Kinesis Stream, it is pretty not helpful to not spread records by shards.

I found that ${newuuid} is suggested in Web Console.
image

This is the documentation of ${newuuid}.

I try to add default value ${newuuid}. But if you have any opinions, please feel free to tell me😉 .

Copy link
Contributor

@skinny85 skinny85 Jan 13, 2022

Choose a reason for hiding this comment

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

I don't hate it, but I'm a little concerned that the L2 won't have all of the capabilities of the underlying service (in this case, leaving the partition key as empty).

Does the Console allow you to leave it as empty, or do you have to provide a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the L2 won't have all of the capabilities of the underlying service

It is so reasonable I think! So I will never mind to revert my commit that add default value.

Does the Console allow you to leave it as empty, or do you have to provide a value?

The Console requre PartitionKey, but CloudFormation does not. For keeping compatibility for CloudFormation, let's not add a default value, instead add a detailed description in JSDoc and Readme?

Copy link
Contributor

@skinny85 skinny85 Jan 14, 2022

Choose a reason for hiding this comment

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

Let's do this.

Make this field required, like it is in the console. But, allow specifying an empty string there (''). If an empty string is passed, then we will actually pass undefined to the underlying CloudFormation resource for that field. Of course, we will need to cover that in our documentation.

This way, we will be close to the Console experience, while covering the entire service capabilities.

What do you think about this @yamatatsu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's good I think! 👍🏻
OK, I will do it!

Comment on lines 18 to 20
errorAction: new actions.CloudWatchLogsAction(
new logs.LogGroup(this, 'Logs', { removalPolicy: cdk.RemovalPolicy.DESTROY }),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it is not needed, but for confirmation that it is work on aws 😅 .
I'll remove it!

@mergify mergify bot dismissed skinny85’s stale review January 11, 2022 03:35

Pull request has been modified.

mergify bot pushed a commit that referenced this pull request Jan 11, 2022
…on` (#18356)

By #18321 (comment)

BREAKING CHANGE: the class `FirehoseStreamAction` has been renamed to `FirehosePutRecordAction`

----

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

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @yamatatsu, thanks for your patience in seeing this one through to the end!

@mergify
Copy link
Contributor

mergify bot commented Jan 18, 2022

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 103df62
  • 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 1480213 into aws:master Jan 19, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2022

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

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…on` (aws#18356)

By aws#18321 (comment)

BREAKING CHANGE: the class `FirehoseStreamAction` has been renamed to `FirehosePutRecordAction`

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
Fixes aws#17703

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@yamatatsu yamatatsu deleted the aws-iot-actions-kinesis branch April 6, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iot Related to AWS IoT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(iot): TopicRule action for Kinesis Data Streams in AWS IoT
3 participants