kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <matth...@confluent.io>
Subject Re: [DISCUSS] KIP-378: Enable Dependency Injection for Kafka Streams handlers
Date Fri, 19 Oct 2018 16:50:44 GMT
I just realized that there is not Jira for this KIP. Can you please
create one? All KIPs need to have a Jira for tracking.

Thanks.

On 10/10/18 6:51 AM, 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 <wangguoz@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 <john@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 <john@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 <wangguoz@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 <dongjin@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.dev@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
>>
> 


Mime
View raw message