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

Spike on a new API for Kafka Streams support #34

Open
wants to merge 4 commits into
base: 5.4.x
Choose a base branch
from

Conversation

bobby
Copy link

@bobby bobby commented May 16, 2019

My team ran into some issues when using KafkaStreams with Micronaut on a current project:

  • Returning KStream from factory methods seemed odd and arbitrary (what if my topology only uses KTable or GlobalKTable ?), and the KStream bean itself is never used by the Factory
  • Running multiple KafkaStreams jobs never quite worked right, since the ConfiguredStreamBuilder and KStream weren't named or specified in the KafkaStreams factory method

So we reworked the API a bit, and this new implementation has solved these issues. Now the API looks like:

    @Named("myStream")
    @Singleton
    fun myStream(@Named("my-stream") configuration: KafkaStreamsConfiguration) : ConfiguredStreamBuilder {
        val builder = ConfiguredStreamBuilder(configuration)
        ...
        return builder
    }

If this API looks good to the maintainers, I'll be happy to update the documentation, etc.

@CLAassistant
Copy link

CLAassistant commented May 16, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ bobby
❌ tetriscode
You have signed the CLA already but the status is still pending? Let us recheck it.

@graemerocher
Copy link
Contributor

Test failures seem unrelated and resolved by another PR

@graemerocher
Copy link
Contributor

The changes seem ok to me, only think is they are breaking changes. I wonder if there is way to maintain some form of backward's compatibility. Either way we need to alter the version in https://github.com/micronaut-projects/micronaut-kafka/blob/master/gradle.properties#L1 to 1.2.0.BUILD-SNAPSHOT

@bobby
Copy link
Author

bobby commented May 18, 2019

@graemerocher If we're backwards-breaking, wouldn't we bump to 2.0.0.BUILD-SNAPSHOT, as per SemVer?

@bobby
Copy link
Author

bobby commented May 18, 2019

I've rebased with my correct email address in the commit author, to please the CLA bot.

@ctoestreich
Copy link
Contributor

@bobby Would there be any doc updates for your changes?

@bobby
Copy link
Author

bobby commented May 21, 2019

@ctoestreich Yes, there will need to be doc changes given the new API. This new API will also enable Kafka Streams testing, but I may add that plus testing docs in its own PR.

@graemerocher
Copy link
Contributor

@bobby could you rebase the changes and provide documentation? We do use semantic versioning so I will need to split out a 1.1.x branch before merging this commit. Thanks.

@graemerocher
Copy link
Contributor

@bobby any more thoughts on documentation? I am thinking that may we should just bump the kafka streams module to 2.0 or even split it into a separate repo. That way the core library can continue to evolve. Thoughts?

@bobby
Copy link
Author

bobby commented May 27, 2019 via email

@bobby
Copy link
Author

bobby commented Jun 6, 2019

While testing an application with several instances of KafkaStreams, we've run up against some difficulties. I'm going to attempt to resolve these difficulties via a few more minor API tweaks in the coming days.

@ctoestreich
Copy link
Contributor

@bobby I also ran into this a bit recently. I am available to help with your branch a bit if you want to resurrect this PR. I spent some time during my last PR to add health trying to dissect the KafkaStreamsFactory as I needed stuff there to relate a kstream to the configuration used to build it for metadata on the health endpiont.

@graemerocher
Copy link
Contributor

@ctoestreich let me know if you plan to contribute this, since we plan to do a release soon. I have updated the module to Kafka 2.5.0 in the meantime

@ctoestreich
Copy link
Contributor

Spending some time today revisiting @bobby code. @bobby @tetriscode what we’re the remaining open issues?

@rgonciarz
Copy link

Hi @bobby @graemerocher @ctoestreich and others, is there any chance to fix this PR and merge it? or to do a serious changes in Micronaut's Kafka Streams configuration/DSL?
I'm trying to escape from Spring stack. Micronaut is very promising but the current support for Kafka Streams is far away from being perfect. What I like with Spring approach is that you may easily create several typologies within one project by exposing simple bean bi/functions/consumers using their Spring Cloud Kafka Streams binder (functional style). Except that they offer a configuration hell in yaml files and you loose control over the streams.
Recently I've noticed a new library with very promising API/DSL on the market that is trying to simplify Kafka Streams configuration: https://www.azkarrastreams.io/docs/getting-started/
I didn't have a chance to try it, but it seems to introduce a lot of dependencies.
I hope you may inspire from that.

@graemerocher
Copy link
Contributor

We would love contributions in this area to advance this PR and make Micronaut with Kafka streams a better experience

@rgonciarz
Copy link

@graemerocher I can't promise now. I have a bigger POC project (btw. a major part might be written in Micronaut) I need to finish till autumn, but who knows after that. I'm also involved in a bigger system written from scratch using Kafka Streams mostly.
At the moment I may give you a quickly you a feedback regarding documentation, especially KS once.
I would add:

  • more examples (e.g. with joins)
  • multiple topologies (with different applicationId)
  • different serialization (example with Avro or protobuf)
  • how to setup security or add tracing
  • health check (e.g. how to ignore false positive related with rebalancing)
    etc., etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants