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(microservice) Kafka Support #2361 #2735

Merged
merged 76 commits into from
Sep 4, 2019

Conversation

mkaufmaner
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

#2361

Issue Number: #2361

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@mkaufmaner mkaufmaner changed the title 2361 kafka microservice WIP feat(microservice) Kafka Support #2361 Aug 12, 2019
@mkaufmaner
Copy link
Contributor Author

Whoops, looks like I messed up some dependencies. Fixing now.

@mkaufmaner mkaufmaner changed the title WIP feat(microservice) Kafka Support #2361 feat(microservice) Kafka Support #2361 Aug 12, 2019
@mkaufmaner
Copy link
Contributor Author

@kamilmysliwiec I have no illusions that this is complete. However, your input would be greatly appreciated.

.npmrc Outdated Show resolved Hide resolved
@kamilmysliwiec
Copy link
Member

Can you elaborate on how the communication is being handled right now?

@kamilmysliwiec kamilmysliwiec added this to the 6.7.0 milestone Aug 26, 2019
@mkaufmaner
Copy link
Contributor Author

@kamilmysliwiec would love to have this included in 6.6.0. Let me know what I have to do to make that happen and I will.

@Hexenon
Copy link
Contributor

Hexenon commented Aug 27, 2019

@mkaufmaner is it ready?, do you need any other help?, i hope my help was enough.

can't wait to use it, i need this Kafka message queue for my application.

Also, i think we need to add a how to use this to docs repo

@kamilmysliwiec
Copy link
Member

We still have to cover these 2 things:

@mkaufmaner
Copy link
Contributor Author

@kamilmysliwiec Alright, I'll merge 6.6.0 into this branch, integrate the serializer/deserializers and work on the documentation.

…-6-6-0

# Conflicts:
#	package-lock.json
#	packages/microservices/interfaces/index.ts
#	packages/microservices/test/listeners-controller.spec.ts
@mkaufmaner
Copy link
Contributor Author

@kamilmysliwiec Should I extend the the serializer/deserializers with Kafka specific serializers (ie. incoming-request-kafka.deserializer.ts)? Or should I implement the parsing of kafka message elsewhere?

@kamilmysliwiec
Copy link
Member

The key responsibility of serializers/deserializers is to adjust incoming/outgoing packages to the unique format required by NestJS. In the best-case scenario, you should use the exact same serializers for Kafka as we use for any other strategy (since Kafka should be considered as an official strategy eventually)

@mkaufmaner
Copy link
Contributor Author

The key responsibility of serializers/deserializers is to adjust incoming/outgoing packages to the unique format required by NestJS. In the best-case scenario, you should use the exact same serializers for Kafka as we use for any other strategy (since Kafka should be considered as an official strategy eventually)

Understandable. However, I don't really see any other way. There is the problem where message keys, values, and headers are buffers which would require the developer to implement their own serialization within their controllers. Subsequently, when a developer changes transports, they would have to modify their controllers significantly. There is also a problem maintaining interoperability within the cluster if we utilize the existing serializers/deserializers. This is because message headers are utilized for message metadata like correlation ids, reply topics, and reply partitions.

@kamilmysliwiec
Copy link
Member

@mkaufmaner unfortunately, I can't tell you more before I dive into the code :( I'll try to do review it as soon as possible

@mkaufmaner
Copy link
Contributor Author

@kamilmysliwiec I see that in some servers like server-mqtt.ts and server-redis.ts you have a method for actually parsing the raw message (parseMessage). I would think this would be handled by the serializer. Is this correct?

@kamilmysliwiec
Copy link
Member

@mkaufmaner actually, no. That's the reason why transport-related parsing is located directly in classes, whereas high-level serialization/deserialization (alignment to external/internal systems) is being done in dedicated serializers/deserializers. Hence, they can be shared among different transport strategies by default.

@mkaufmaner
Copy link
Contributor Author

@mkaufmaner actually, no. That's the reason why transport-related parsing is located directly in classes, whereas high-level serialization/deserialization (alignment to external/internal systems) is being done in dedicated serializers/deserializers. Hence, they can be shared among different transport strategies by default.

@kamilmysliwiec Gotcha.

@mkaufmaner mkaufmaner closed this Aug 28, 2019
@mkaufmaner mkaufmaner reopened this Aug 28, 2019
@mkaufmaner
Copy link
Contributor Author

@kamilmysliwiec I implemented the serializers/deserializers. However, can't seem to get the CircleCI builds to run.

@kamilmysliwiec kamilmysliwiec merged commit 1162503 into nestjs:master Sep 4, 2019
@kamilmysliwiec
Copy link
Member

Thank you @mkaufmaner. I have adjusted performed changes to the current codebase style and merged PR. I hope to publish this feature within 6.7.0 release.

@wejson
Copy link

wejson commented Sep 19, 2019

I c this is released, Thank u :)

Is there any documentation? @mkaufmaner
i really looking forward to using this with the cqrs

@vjtc0n
Copy link

vjtc0n commented Nov 9, 2019

@kamilmysliwiec Hello, Great work!
But it's November now, can we have an example to work with?
Thanks.

@lock
Copy link

lock bot commented Feb 8, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants