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.

Sign up for GitHub

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

Jump to bottom

feat(lambda-event-sources): msk and self-managed kafka event sources #12507

Merged
merged 59 commits into from
Mar 16, 2021
Merged

feat(lambda-event-sources): msk and self-managed kafka event sources #12507

merged 59 commits into from
Mar 16, 2021

Conversation

bracki
Copy link
Contributor

@bracki bracki commented Jan 14, 2021

Fixes #12099

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 •
edited
Loading

@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2021

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

@bracki bracki changed the title (lambda-event-sources): Implement Lambda event source for Kafka feat(lambda-event-sources): Implement Lambda event source for Kafka Jan 14, 2021
@bracki bracki force-pushed the kafka-lambda-event-source branch 3 times, most recently from c6e5881 to 92e6592 Compare January 18, 2021 11:56
nija-at
nija-at previously requested changes Jan 18, 2021
View reviewed changes
Copy link
Contributor

@nija-at nija-at 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 submitting this PR! 😊
First round of comments below.

packages/@aws-cdk/aws-lambda-event-sources/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/test/test.kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed nija-at’s stale review January 18, 2021 13:12

Pull request has been modified.

@bracki bracki requested a review from nija-at January 19, 2021 16:57
@bracki
Copy link
Contributor Author

bracki commented Jan 19, 2021 •
edited
Loading

I think I addressed your feedback @nija-at. EventSourceMappingOptions is a bit bloated now, but I don't see the benefit of flattening everything as mentioned in the design docs, because I believe you/somebody shouldn't use this directly. That's what the event sources are for.

@nija-at
Copy link
Contributor

nija-at commented Jan 25, 2021

Hey @bracki - sorry for the delay to get back to this.
I had a very busy last week, and I'm afraid this week is going to be the same. I'll get back on this PR as soon as I'm able (likely early next week).

Thanks for your patience.

@bracki bracki force-pushed the kafka-lambda-event-source branch 2 times, most recently from 26668b7 to d40bc2c Compare February 4, 2021 09:22
nija-at
nija-at previously requested changes Feb 5, 2021
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Hey @bracki - Sorry to have left this languishing. I've taken another look and comments are below.

packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/stream.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
@nija-at nija-at changed the title feat(lambda-event-sources): Implement Lambda event source for Kafka feat(lambda-event-sources): msk and self managed kafka event sources Feb 5, 2021
@nija-at nija-at changed the title feat(lambda-event-sources): msk and self managed kafka event sources feat(lambda-event-sources): msk and self-managed kafka event sources Feb 5, 2021
@mergify mergify bot dismissed nija-at’s stale review February 11, 2021 19:12

Pull request has been modified.

@bracki
Copy link
Contributor Author

bracki commented Feb 11, 2021

@nija-at All feedback addressed, apart from the JSII thing and the ICluster.

nija-at
nija-at previously requested changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the changes @bracki. See my comments below.

Could you address - #12507 (comment)

packages/@aws-cdk/aws-lambda-event-sources/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-event-sources/test/test.kafka.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/event-source-mapping.ts Outdated Show resolved Hide resolved
@gitpod-io
Copy link

gitpod-io bot commented Feb 16, 2021 •
edited
Loading

@mergify mergify bot dismissed nija-at’s stale review February 16, 2021 16:28

Pull request has been modified.

@mergify mergify bot dismissed nija-at’s stale review March 16, 2021 09:50

Pull request has been modified.

nija-at
nija-at approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Awesome!

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 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).

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 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).

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 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).

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 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).

@mergify mergify bot merged commit 73209e1 into aws:master Mar 16, 2021
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Mar 17, 2021
@bracki @hollanddd
feat(lambda-event-sources): msk and self-managed kafka event sources (a…
99d83e9 
…ws#12507)

Fixes aws#12099 
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Mar 18, 2021
@bracki @hollanddd
feat(lambda-event-sources): msk and self-managed kafka event sources (a…
1959401 
…ws#12507)

Fixes aws#12099 
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
@bracki @hollanddd
feat(lambda-event-sources): msk and self-managed kafka event sources (a…
6ab4f1f 
…ws#12507)

Fixes aws#12099 
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwt-janbrauer iwt-janbrauer iwt-janbrauer left review comments

@nija-at nija-at nija-at approved these changes

Assignees

@nija-at nija-at

Labels
@aws-cdk/aws-lambda-event-sources
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(lambda-event-sources): Support MSK and self hosted Kafka
4 participants
@bracki @nija-at @aws-cdk-automation @iwt-janbrauer