Skip to content

Add support for Kinesis Data Streams Enhanced Fan-out (Consumers)#7320

Merged
medikoo merged 5 commits into
serverless:masterfrom
zaccharles:kinesis-consumer
Feb 14, 2020
Merged

Add support for Kinesis Data Streams Enhanced Fan-out (Consumers)#7320
medikoo merged 5 commits into
serverless:masterfrom
zaccharles:kinesis-consumer

Conversation

@zaccharles

@zaccharles zaccharles commented Feb 11, 2020

Copy link
Copy Markdown
Contributor

What did you implement

Closes #5510

I added the a new configuration property for Kinesis data stream events which configures the event source mapping to use a stream consumer instead of shard iterator.

It creates a new stream consumer when set to true and uses an existing consumer if given the ARN of one.

It manages the necessary IAM permissions and supports functions such as Ref and ImportValue (essential since stream consumer ARNs are AWS generated with timestamps in them).

This is my first PR of this size to this project, so apologies if I didn't quite get some conventions right.

How can we verify it

service: dct

provider:
  name: aws
  runtime: nodejs12.x

functions:
  hello:
    handler: handler.hello
    events:
      - stream:
          batchSize: 1
          type: kinesis
          arn:
            Fn::GetAtt:
              - ExampleStream
              - Arn
          consumer: true
      - stream:
          batchSize: 1
          type: kinesis
          arn:
            Fn::GetAtt:
              - ExampleStream
              - Arn
          consumer:
            Ref: ExampleStreamConsumer

resources:
  Resources:
    ExampleStream: 
      Type: AWS::Kinesis::Stream 
      Properties: 
          ShardCount: 1
    ExampleStreamConsumer: 
      Type: AWS::Kinesis::StreamConsumer
      Properties: 
          StreamARN:
            Fn::GetAtt:
              - ExampleStream
              - Arn
          ConsumerName: whatever

Todos

No todos at the moment.

  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

medikoo
medikoo suggested changes Feb 12, 2020
View reviewed changes

@medikoo medikoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great thanks @zaccharles It looks really good.

I've proposed few improvements, let me know what you think

Comment thread lib/plugins/aws/package/compile/events/stream/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/stream/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/stream/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/stream/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/stream/index.js Outdated
Comment thread lib/plugins/aws/package/compile/events/stream/index.test.js Outdated
@zaccharles

zaccharles commented Feb 12, 2020

Copy link
Copy Markdown
Contributor Author

@medikoo Hi, thanks for the review. I've made some more changes. Specifically, I've relied more on CloudFormation validating things, which is probably a good approach. This has allowed me to simplify things a bit. I'm happy to accept more feedback.

@codecov-io

codecov-io commented Feb 12, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #7320 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7320   +/-   ##
=======================================
  Coverage   87.91%   87.91%           
=======================================
  Files         240      240           
  Lines        8888     8888           
=======================================
  Hits         7814     7814           
  Misses       1074     1074
Impacted Files Coverage Δ
lib/plugins/aws/package/lib/mergeIamTemplates.js 100% <100%> (ø) ⬆️
...ckage/compile/events/apiGateway/lib/permissions.js 88.23% <100%> (ø) ⬆️
.../package/compile/events/apiGateway/lib/validate.js 96.94% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73fc5a3...f8fb2a5. Read the comment docs.

medikoo
medikoo suggested changes Feb 12, 2020
View reviewed changes

@medikoo medikoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @zaccharles looks really good now, just added one extra suggestion.

Comment thread lib/plugins/aws/package/compile/events/stream/index.js Outdated
@zaccharles

zaccharles commented Feb 13, 2020

Copy link
Copy Markdown
Contributor Author

It's quite a lot simpler now without any of the validation 😄

medikoo
medikoo approved these changes Feb 14, 2020
View reviewed changes

@medikoo medikoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, thank you @zaccharles !

@medikoo
medikoo merged commit 9eba218 into serverless:master Feb 14, 2020
@snyk-bot snyk-bot mentioned this pull request Mar 15, 2020
Merged
astuyve pushed a commit that referenced this pull request Apr 15, 2020
@zaccharles @astuyve
feat: Support Kinesis Enhanced Fan-out (Consumer) streams (#7320)
a5860ef
@xharsin

xharsin commented Jun 16, 2020

Copy link
Copy Markdown

@zaccharles Is it possible to use the function name defined with name in serverless under function.

 functions:
   preprocess:
     name: preprocess-test
     handler: handler.preprocess
     events:
       - stream:
           arn: arn:aws:kinesis:region:XXXXXX:stream/foo
           consumer: true

The Kinesis consumer is created with functionNamestreamNameConsumer as naming but it is getting the functionName as preprocess and not preprocess-test

I my use case I need to deploy multiple functions with stages name so I am adding stage name in functions names using name

@suparngp suparngp mentioned this pull request Oct 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@medikoo medikoo medikoo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@zaccharles zaccharles

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for AWS Kinesis enhanced stream consumer for Lambda

4 participants

@zaccharles @codecov-io @xharsin @medikoo