John, thanks for the explanation, now it makes much more sense to me. As for the concrete approach, to me it seems the first option requires less changes than the second (ConfiguredStreamsFactory based) approach, whereas the second one requires an additional interface that is overlapping with the AbstractConfig.
I'm aware that in KafkaProducer / KafkaConsumer we do not have public constructors for taking a ProducerConfig or ConsumerConfig directly, and anyone using Spring can share how you've worked around it by far? If it is very awkward I'm not against just adding the XXXConfigs to the constructors directly. Guozhang On Fri, Oct 5, 2018 at 1:48 PM, John Roesler <j...@confluent.io> wrote: > Hi Wladimir, > > Thanks for the KIP! > > As I mentioned in the PR discussion, I personally prefer not to recommend > overriding StreamsConfig for this purpose. > > It seems like a person wishing to create a DI shim would have to acquire > quite a deep understanding of the class and its usage to figure out what > exactly to override to accomplish their goals without breaking everything. > I'm honestly impressed with the method you came up with to create your > Spring/Streams shim. > > I think we can make to path for the next person smoother by going with > something more akin to the ConfiguredStreamsFactory. This is a constrained > interface that tells you exactly what you have to implement to create such > a shim. > > A few thoughts: > 1. it seems like we can keep all the deprecated constructors still > deprecated > > 2. we could add just one additional constructor to each of KafkaStreams and > TopologyTestDriver to still take a Properties, but also your new > ConfiguredStreamsFactory > > 3. I don't know if I'm sold on the name ConfiguredStreamsFactory, since it > does not produce configured streams. Instead, it produces configured > instances... How about ConfiguredInstanceFactory? > > 4. if I understand the usage correctly, it's actually a pretty small number > of classes that we actually make via getConfiguredInstance. Offhand, I can > think of the key/value Serdes, the deserialization exception handler, and > the production exception handler. > Perhaps, instead of maintaining a generic "class instantiator", we could > explore a factory interface that just has methods for creating exactly the > kinds of things we need to create. In fact, we already have something like > this: org.apache.kafka.streams.KafkaClientSupplier . Do you think we could > just add some more methods to that interface (and maybe rename it) instead? > > Thanks, > -John > > On Fri, Oct 5, 2018 at 3:31 PM John Roesler <j...@confluent.io> wrote: > > > Hi Guozhang, > > > > I'm going to drop in a little extra context from the preliminary PR > > discussion (https://github.com/apache/kafka/pull/5344). > > > > The issue isn't that it's impossible to use Streams within a Spring app, > > just that the interplay between our style of construction/configuration > and > > Spring's is somewhat awkward compared to the normal experience with > > dependency injection. > > > > I'm guessing users of dependency injection would not like the approach > you > > offered. I believe it's commonly considered an antipattern when using DI > > frameworks to pass the injector directly into the class being > constructed. > > Wladimir has also offered an alternative usage within the current > framework > > of injecting pre-constructed dependencies into the Properties, and then > > retrieving and casting them inside the configured class. > > > > It seems like this KIP is more about offering a more elegant interface to > > DI users. > > > > One of the points that Wladimir raised on his PR discussion was the > desire > > to configure the classes in a typesafe way in the constructor (thus > > allowing the use of immutable classes). > > > > With this KIP, it would be possible for a DI user to: > > 1. register a Streams-Spring or Streams-Guice (etc) "plugin" (via either > > of the mechanisms he proposed) > > 2. simply make the Serdes, exception handlers, etc, available on the > class > > path with the DI annotations > > 3. start the app > > > > There's no need to mess with passing dependencies (or the injector) > > through the properties. > > > > Sorry for "injecting" myself into your discussion, but it took me a while > > in the PR discussion to get to the bottom of the issue, and I wanted to > > spare you the same. > > > > I'll respond separately with my feedback on the KIP. > > > > Thanks, > > -John > > > > On Sun, Sep 30, 2018 at 2:31 PM Guozhang Wang <wangg...@gmail.com> > wrote: > > > >> Hello Wladimir, > >> > >> Thanks for proposing the KIP. I think the injection can currently be > done > >> by passing in the key/value pair directly into the properties which can > >> then be accessed from the `ProcessorContext#appConfigs` or > >> `#appConfigsWithPrefix`. For example, when constructing the properties > you > >> can: > >> > >> ``` > >> props.put(myProp1, myValue1); > >> props.put(myProp2, myValue1); > >> props.put("my_app_context", appContext); > >> > >> KafkaStreams myApp = new KafkaStreams(topology, props); > >> > >> // and then in your processor, on the processor where you want to > >> construct > >> the injected handler: > >> > >> Map<String, Object> appProps = processorContext.appConfigs(); > >> ApplicationContext appContext = appProps.get("my_app_context"); > >> MyHandler myHandler = > >> applicationContext.getBeanNamesForType(MyHandlerClassType); > >> ``` > >> > >> Does that work for you? > >> > >> Guozhang > >> > >> > >> On Sun, Sep 30, 2018 at 6:56 AM, Dongjin Lee <dong...@apache.org> > wrote: > >> > >> > Hi Wladimir, > >> > > >> > Thanks for your great KIP. Let me have a look. And let's discuss this > >> KIP > >> > in depth after the release of 2.1.0. (The committers are very busy for > >> it.) > >> > > >> > Best, > >> > Dongjin > >> > > >> > On Sun, Sep 30, 2018 at 10:49 PM Wladimir Schmidt <wlsc....@gmail.com > > > >> > wrote: > >> > > >> > > Dear colleagues, > >> > > > >> > > I am happy to inform you that I have just finished my first KIP > >> > > (KIP-378: Enable Dependency Injection for Kafka Streams handlers > >> > > < > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > >> > 378%3A+Enable+Dependency+Injection+for+Kafka+Streams+handlers > >> > > >). > >> > > > >> > > Your feedback on this submission would be highly appreciated. > >> > > > >> > > Best Regards, > >> > > Wladimir Schmidt > >> > > > >> > > >> > > >> > -- > >> > *Dongjin Lee* > >> > > >> > *A hitchhiker in the mathematical world.* > >> > > >> > *github: <http://goog_969573159/>github.com/dongjinleekr > >> > <http://github.com/dongjinleekr>linkedin: > >> kr.linkedin.com/in/dongjinleekr > >> > <http://kr.linkedin.com/in/dongjinleekr>slideshare: > >> > www.slideshare.net/dongjinleekr > >> > <http://www.slideshare.net/dongjinleekr>* > >> > > >> > >> > >> > >> -- > >> -- Guozhang > >> > > > -- -- Guozhang
