Hi Guozhang, yes i think it would make sense to add this now as having the
additional record context would be valuable. Though i'm happy either way.
On Wed, 16 May 2018 at 23:07 Guozhang Wang <wangg...@gmail.com> wrote:
> Hi Damian,
>
> My current plan is to add the "RichKeyValueMapper" when getting in KIP-159
> to include the record context in this dynamic routing feature. So just to
> clarify: are you more concerning that if we are going to do that anyways in
> the future, we should not add overloaded functions with "KeyValueMapper" as
> of now since they will be subsumed soon?
>
>
> Guozhang
>
>
> On Wed, May 16, 2018 at 2:59 PM, Damian Guy <damian....@gmail.com> wrote:
>
>> Overall i'm a +1 on this, but i'm not a big fan of using the
>> KeyValueMapper
>> to choose the topic. It is a bit counter-intuitve to me. I'd prefer to add
>> a class specifically for it and possibly pass in the RecordContext
>>
>> On Wed, 16 May 2018 at 13:22 Guozhang Wang <wangg...@gmail.com> wrote:
>>
>> > Hello folks,
>> >
>> > Please let me know if you have further feedbacks; if there is no more
>> > feedbacks I'm going to start the voting thread soon.
>> >
>> >
>> > Guozhang
>> >
>> >
>> > On Wed, May 16, 2018 at 8:31 AM, Guozhang Wang <wangg...@gmail.com>
>> wrote:
>> >
>> > > I have thought about exposing record context as well, and in the end I
>> > > decided to piggy-back it with KIP-159. And if we want to indeed reuse
>> the
>> > > class it will be:
>> > >
>> > > ```
>> > > public interface RichKeyValueMapper<K, V, VR> {
>> > > VR apply(final K key, final V value, final RecordContext
>> > > recordContext);
>> > > }
>> > > ```
>> > >
>> > >
>> > >
>> > > Guozhang
>> > >
>> > >
>> > > On Tue, May 15, 2018 at 10:04 PM, Matthias J. Sax <
>> matth...@confluent.io
>> > >
>> > > wrote:
>> > >
>> > >> Just my 2 cents:
>> > >>
>> > >> I am fine with `KeyValueMapper` (+1 for code reusage) -- the JavaDocs
>> > >> will explain what the `KeyValueMapper` is supposed to do, ie, extract
>> > >> and return the sink topic name from the key-value pair.
>> > >>
>> > >> A side remark though: do we think that accessing key/value is
>> > >> sufficient? Or should we provide access to the full metadata? We
>> could
>> > >> also do this with KIP-159 of course -- but this would come earliest
>> in
>> > >> 2.1. As an alternative we could add a `TopicNameExtractor` to expose
>> the
>> > >> whole record context. The advantage would be, that we don't need to
>> > >> change it via KIP-159 later again. WDYT?
>> > >>
>> > >> -Matthias
>> > >>
>> > >> On 5/15/18 5:57 PM, Bill Bejeck wrote:
>> > >> > Thanks for the KIP Guozhang, it's a +1 for me.
>> > >> >
>> > >> > As for re-using the KeyValueMapper for choosing the topic, I am on
>> the
>> > >> > fence, a more explicitly named class would be more clear, but I'm
>> not
>> > >> sure
>> > >> > it's worth a new class that will primarily perform the same
>> actions as
>> > >> the
>> > >> > KeyValueMapper.
>> > >> >
>> > >> > Thanks,
>> > >> > Bill
>> > >> >
>> > >> > On Tue, May 15, 2018 at 5:52 PM, Guozhang Wang <wangg...@gmail.com
>> >
>> > >> wrote:
>> > >> >
>> > >> >> Hello John:
>> > >> >>
>> > >> >> * As for the type superclass, it is mainly for allowing super
>> class
>> > >> serdes.
>> > >> >> More details can be found here:
>> > >> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > >> >> 100+-+Relax+Type+constraints+in+Kafka+Streams+API
>> > >> >>
>> > >> >> * I may have slight preference on reusing existing classes but I
>> > think
>> > >> most
>> > >> >> of my rationales are quite subjective. Personally I do not find
>> `self
>> > >> >> documenting` worth a new class but I can be convinced if people
>> have
>> > >> other
>> > >> >> motivations doing it :)
>> > >> >>
>> > >> >>
>> > >> >> Guozhang
>> > >> >>
>> > >> >>
>> > >> >> On Tue, May 15, 2018 at 11:19 AM, John Roesler <j...@confluent.io
>> >
>> > >> wrote:
>> > >> >>
>> > >> >>> Thanks for the KIP, Guozhang.
>> > >> >>>
>> > >> >>> It looks good overall to me; I just have one question:
>> > >> >>> * Why do we bound the generics of KVMapper in KStream to be
>> > >> >> superclass-of-K
>> > >> >>> and superclass-of-V instead of exactly K and V, as in Topology? I
>> > >> might
>> > >> >> be
>> > >> >>> thinking about it wrong, but that seems backwards to me. If
>> > anything,
>> > >> >>> bounding to be a subclass-of-K or subclass-of-V would seem right
>> to
>> > >> me.
>> > >> >>>
>> > >> >>> One extra thought: I agree that KVMapper<K,V,String /*topic
>> name*/>
>> > >> is an
>> > >> >>> applicable type for extracting the topic name, but I wonder what
>> the
>> > >> >> value
>> > >> >>> of reusing the KVMapper for this purpose is. Would defining a new
>> > >> class,
>> > >> >>> say TopicNameExtractor<K,V>, provide the same functionality while
>> > >> being a
>> > >> >>> bit more self-documenting?
>> > >> >>>
>> > >> >>> Thanks,
>> > >> >>> -John
>> > >> >>>
>> > >> >>> On Tue, May 15, 2018 at 12:32 AM, Guozhang Wang <
>> wangg...@gmail.com
>> > >
>> > >> >>> wrote:
>> > >> >>>
>> > >> >>>> Hello folks,
>> > >> >>>>
>> > >> >>>> I'd like to start a discussion on adding dynamic routing
>> > >> functionality
>> > >> >> in
>> > >> >>>> Streams sink node. I.e. users do not need to specify the topic
>> name
>> > >> at
>> > >> >>>> compilation time but can dynamically determine which topic to
>> send
>> > to
>> > >> >>> based
>> > >> >>>> on each record's key value pairs. Please find a KIP here:
>> > >> >>>>
>> > >> >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > >> >>>> 303%3A+Add+Dynamic+Routing+in+Streams+Sink
>> > >> >>>>
>> > >> >>>> Any feedbacks are highly appreciated.
>> > >> >>>>
>> > >> >>>> Thanks!
>> > >> >>>>
>> > >> >>>> -- Guozhang
>> > >> >>>>
>> > >> >>>
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >> --
>> > >> >> -- Guozhang
>> > >> >>
>> > >> >
>> > >>
>> > >>
>> > >
>> > >
>> > > --
>> > > -- Guozhang
>> > >
>> >
>> >
>> >
>> > --
>> > -- Guozhang
>> >
>>
>
>
>
> --
> -- Guozhang
>
