Re: [DISCUSS] KIP-378: Enable Dependency Injection for Kafka Streams handlers

["Matthias J. Sax" <ma...@confluent.io>]

Moved this KIP into status "inactive". Feel free to resume and any time.

-Matthias

On 4/26/19 2:00 AM, Matthias J. Sax wrote:
> What is the status of this KIP?
> 
> -Matthias
> 
> 
> On 3/18/19 5:11 PM, Guozhang Wang wrote:
>> Hello Wladimir,
>>
>> Thanks for the replies.
>>
>> What do you mean by "the community has opted for the second more complex
>> solution"? Could you elaborate a bit more?
>>
>> Guozhang
>>
>>
>> On Sun, Mar 17, 2019 at 3:45 PM Wladimir Schmidt <wl...@gmail.com> wrote:
>>
>>> Hi Matthias,
>>>
>>> Sorry, due to other commitments I haven't started the other
>>> implementation yet.
>>> In the meantime, the community has opted for the second, more complex
>>> solution.
>>> I already had ideas in this regard, but their elaboration needs to be
>>> discussed more.
>>>
>>>
>>> Best greetings,
>>> Wladimir
>>>
>>>
>>> On 21-Feb-19 09:33, Matthias J. Sax wrote:
>>>> 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 <wa...@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 <wl...@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 <wl...@gmail.com>
>>> <wl...@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 <wa...@gmail.com> <
>>> 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 <jo...@confluent.io> <
>>> 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 <jo...@confluent.io> <
>>> 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 <wa...@gmail.com> <
>>> 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 <do...@apache.org> <
>>> 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/> <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
>>>>>>
>>>>>
>>>
>>
>>
>