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

#2663 Quarkus Extension for Kafka Streams #2693

Merged
merged 6 commits into from
Jun 11, 2019

Conversation

gunnarmorling
Copy link
Contributor

@gunnarmorling gunnarmorling commented Jun 2, 2019

Planning some more testing under native, but the basic functionality is there.
Feedback welcome!
Ready for (re-)review from my side.

@gunnarmorling gunnarmorling changed the title #2663 Kafka Streams #2663 Quarkus Extension for Kafka Streams Jun 2, 2019
@kenfinnigan
Copy link
Member

I'm quite the noob when it comes to Kafka.

How does this differ from Reactive Messaging with Kafka?

@gunnarmorling
Copy link
Contributor Author

Kafka Streams is used for implementing streaming queries based on top of Kafka, like mapping and filtering topics, grouping/aggregating values, joining topics, windowing etc. It also lets you run interactive queries (querying current state instead of streaming topics), all that in a distributed and scalable fashion.

It's a popular API of the Kafka eco-system (and part of the Kafka project) and having support for it in Quarkus will complete the Kafka story of it. mP Reactive Operators spec can do some of these things, but I don't think it goes as far.

@gunnarmorling
Copy link
Contributor Author

@cescoffier, @gsmet, any feedback? Btw. how can I actually "request a review"? Seems I'd have to be part of the org or something? Thanks!

Btw. I did some more testing locally; interactive queries work, too. I'll push a test case for this. With that, I'd consider it good enough for a first release. Happy to learn about your thoughts.

gsmet
gsmet previously requested changes Jun 3, 2019
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Code looks good. I added a comment about some dependencies that look incorrect and the configuration where I think we need @cescoffier to step in.

BTW, we should probably either extend the existing Kafka guide (it's probably already complex enough) or write another one if we think it's worth advertising this feature.

@gunnarmorling
Copy link
Contributor Author

gunnarmorling commented Jun 3, 2019

Ok, so provided CI passes, I'm happy with this change now. All, please let me know whether you see any issues. Thanks!

@gsmet
Copy link
Member

gsmet commented Jun 4, 2019

I added one additional comment. Otherwise, it looks good code wise.

@cescoffier could you check it's in line with the general strategy you have with Apache Kafka support?

Also a guide/quickstart explaining why it is useful would be nice (let's wait for @cescoffier approval first).

@cescoffier
Copy link
Member

First to answer to @gunnarmorling most of the features are available in MP Reactive Messaging when you use Stream-based functions (generally using RX Java 2, or Reactor) (windowing, groups, join...). The main differences are:

  • it's not Kafka specific
  • it does not back things into Kafka (but just in memory, so it's good and bad at the same time)

Now, I agree that Kafka streams are popular in the Kafka world, and we should have it. For Quarkus the main strategy stays MP Reactive Messaging. This extension should be used if you have a Kafka Streams pipeline and want to migrate to Quarkus.

@gunnarmorling
Copy link
Contributor Author

Thanks for reviewing; I'll update for Awaitility and JSON ordering and force-push then.

@gunnarmorling
Copy link
Contributor Author

Reworked as requested, rebased and force-pushed.

@gunnarmorling
Copy link
Contributor Author

Also added one more commit to move the integration tests into the "kafka" package, a left-over from #2691.

@cescoffier
Copy link
Member

@gunnarmorling I've added one comment around the extension tags. That the last remaining bit.

@cescoffier cescoffier added the triage/on-ice Frozen until external concerns are resolved label Jun 7, 2019
@gunnarmorling
Copy link
Contributor Author

@emmanuelbernard, @cescoffier, did you have a chance to converge on this one during your stand-up yesterday? How should we move forward? I'd like to wrap up this one :) Thanks!

@cescoffier
Copy link
Member

@gunnarmorling Sorry, forgot to add a link to #2737 where we are changing the extension selection algorithm.

As this new algorithm is planned for 0.17.0, I believe we can merge this one with the kafka label.

@cescoffier
Copy link
Member

Just because sometimes basics idea are hard to implement I'm giving a spin to the idea from #2737 and so how it goes...

@gunnarmorling
Copy link
Contributor Author

As this new algorithm is planned for 0.17.0, I believe we can merge this one with the kafka label.

That's great news!

I'm working on a quickstart example for this extension now btw.

@cescoffier
Copy link
Member

Great!

@cescoffier
Copy link
Member

I've opened a PR for the new selection algorithm (#2785). So we can merge this PR.

@cescoffier cescoffier removed the triage/on-ice Frozen until external concerns are resolved label Jun 11, 2019
@cescoffier cescoffier dismissed gsmet’s stale review June 11, 2019 08:52

All comments have been addressed.

@cescoffier cescoffier merged commit cb48004 into quarkusio:master Jun 11, 2019
@gunnarmorling
Copy link
Contributor Author

Yeehaw! Thanks everybody.

@gsmet gsmet added this to the 0.17.0 milestone Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants