Hi Wladimir, what is the status of this KIP?
-Matthias On 1/9/19 4:17 PM, Guozhang Wang wrote: > Hello Wladimir, > > Just checking if you are still working on this KIP. We have the 2.2 KIP > freeze deadline by 24th this month, and it'll be great to complete this KIP > by then so 2.2.0 release could have this feature. > > > Guozhang > > On Mon, Dec 3, 2018 at 11:26 PM Guozhang Wang <wangg...@gmail.com> wrote: > >> Hello Wladimir, >> >> I've thought about the two options and I think I'm sold on the second >> option and actually I think it is better generalize it to be potentially >> used for other clients (producer, consumer) as while since they also have >> similar dependency injection requests for metrics reporter, partitioner, >> partition assignor etc. >> >> So I'd suggest we add the following to AbstractConfig directly (note I >> intentionally renamed the class to ConfiguredInstanceFactory to be used for >> other clients as well): >> >> ``` >> AbstractConfig(ConfigDef definition, Map<?, ?> originals, >> ConfiguredInstanceFactory, boolean doLog) >> ``` >> >> And then in StreamsConfig add: >> >> ``` >> StreamsConfig(Map<?, ?> props, ConfiguredInstanceFactory) >> ``` >> >> which would call the above AbstractConfig constructor (we can leave to >> core team to decide when they want to add for producer and consumer); >> >> And in KafkaStreams / TopologyTestDriver we can add one overloaded >> constructor each that includes all the parameters including the >> ConfiguredInstanceFactory --- for those who only want `factory` but not >> `client-suppliers` for example, they can set it to `null` and the streams >> library will just use the default one. >> >> >> Guozhang >> >> >> On Sun, Dec 2, 2018 at 12:13 PM Wladimir Schmidt <wlsc....@gmail.com> >> wrote: >> >>> Hello Guozhang, >>> sure, the first approach is very straight-forward and allows minimal >>> changes to the Kafka Streams API. >>> On the other hand, second approach with the interface implementation >>> looks more cleaner to me. >>> I totally agree that this should be first discussed before will be >>> implemented. >>> >>> Thanks, Wladimir >>> >>> >>> On 17-Nov-18 23:37, Guozhang Wang wrote: >>> >>> Hello folks, >>> >>> I'd like to revive this thread for discussion. After reading the previous >>> emails I think I'm still a bit leaning towards re-enabling to pass in >>> StreamsConfig to Kafka Streams constructors compared with a >>> ConfiguredStreamsFactory as additional parameters to overloaded >>> KafkaStreams constructors: although the former seems less cleaner as it >>> requires users to read through the usage of AbstractConfig to know how to >>> use it in their frameworks, this to me is a solvable problem through >>> documentations, plus AbstractConfig is a public interface already and hence >>> the additional ConfiguredStreamsFactory to me is really a bit overlapping >>> in functionality. >>> >>> >>> Guozhang >>> >>> >>> >>> On Sun, Oct 21, 2018 at 1:41 PM Wladimir Schmidt <wlsc....@gmail.com> >>> <wlsc....@gmail.com> wrote: >>> >>> >>> Hi Damian, >>> >>> The first approach was added only because it had been initially proposed >>> in my pull request, >>> which started a discussion and thus, the KIP-378 was born. >>> >>> Yes, I would like to have something "injectable". In this regard, a >>> `ConfiguredStreamsFactory` (name is a subject to discussion) >>> is a good option to be introduced into `KafkaStreams` constructor. >>> >>> Even though, I consider the second approach to be cleaner, it involves a >>> certain amount of refactoring of the streams library. >>> The first approach, on the contrary, adds (or removes deprecated >>> annotation, if the method has not been removed yet) only additional >>> constructors with >>> considerably less intervention into a streams library (no changes, which >>> would break an API. Please see a pull >>> request:https://github.com/apache/kafka/pull/5344). >>> >>> Thanks >>> Wladimir >>> >>> On 10-Oct-18 15:51, Damian Guy wrote: >>> >>> Hi Wladimir, >>> >>> Of the two approaches in the KIP - i feel the second approach is cleaner. >>> However, am i correct in assuming that you want to have the >>> `ConfiguredStreamsFactory` as a ctor arg in `StreamsConfig` so that >>> >>> Spring >>> >>> can inject this for you? >>> >>> Otherwise you could just put the ApplicationContext as a property in the >>> config and then use that via the configure method of the appropriate >>> handler to get your actual handler. >>> >>> Thanks, >>> Damian >>> >>> On Tue, 9 Oct 2018 at 01:55, Guozhang Wang <wangg...@gmail.com> >>> <wangg...@gmail.com> wrote: >>> >>> >>> 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> >>> <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> >>> <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> >>> <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> >>> <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/> >>> <http://goog_969573159/>github.com/dongjinleekr<http://github.com/dongjinleekr> >>> <http://github.com/dongjinleekr>linkedin: >>> >>> kr.linkedin.com/in/dongjinleekr >>> >>> <http://kr.linkedin.com/in/dongjinleekr> >>> <http://kr.linkedin.com/in/dongjinleekr>slideshare:www.slideshare.net/dongjinleekr<http://www.slideshare.net/dongjinleekr> >>> <http://www.slideshare.net/dongjinleekr>* >>> >>> >>> -- >>> -- Guozhang >>> >>> >>> -- >>> -- Guozhang >>> >>> >>> >> >> -- >> -- Guozhang >> > >
signature.asc
Description: OpenPGP digital signature
