Thanks Matthias, will do.
On Mon, May 21, 2018 at 3:35 PM, Matthias J. Sax <matth...@confluent.io>
wrote:
> Thanks. I am fine with changing the `StreamPartitioner` interface directly.
>
> Can you add the idea bout `DynamicStreamPartitioner` to "rejected
> alternative" section. Thx.
>
>
> recasting +1 (binding)
>
>
> -Matthias
>
>
> On 5/21/18 3:04 PM, Guozhang Wang wrote:
> > Hello Matthias,
> >
> > I've tried it out on the PR, the implementation should be fine but one
> > concern I had is that, as you may also realize users of
> > DynamicStreamPartitioner needs to implement two interface functions, with
> > and without the topic name if it is extending from StreamPartitioner; we
> > could also let it to not extend from StreamPartition so it has one
> function
> > only but then we'd need Produced to have two functions allowing
> > StreamPartitioner and DynamicStreamPartitioner. Thinking about the pros
> and
> > cons I'm think it may be better to just change the interface of
> > StreamPartitioner itself, since even without dynamic routing, allowing
> the
> > topic name could let users to give one partitioner implementation that
> > branch on the topic names other than having one partitioner per topic.
> >
> >
> > Guozhang
> >
> >
> > On Mon, May 21, 2018 at 11:56 AM, Matthias J. Sax <matth...@confluent.io
> >
> > wrote:
> >
> >> I think that the risk of the change is moderate as I expect most people
> >> to use the DefaultStreamPartitioner.
> >>
> >> However, there would still be possibility to define a new interface
> >> instead of changing the old:
> >>
> >>> public interface DynamicStreamPartitioner<K, V> {
> >>> Integer partition(String topic, K key, V value, int numPartitions);
> >>> }
> >>
> >> The newly added methods `Topology#addSink` and `KStream#to` would take
> >> this new interface instead of the old.
> >>
> >> Maybe `DynamicStreamPartitioner` must extend `StreamPartitioner` to make
> >> runtime code work though...
> >>
> >> WDYT?
> >>
> >> -Matthias
> >>
> >> On 5/21/18 11:47 AM, Guozhang Wang wrote:
> >>> Hello everyone,
> >>>
> >>> While implementing the PR for this KIP I realized there is once place
> >> which
> >>> we should consider modifying on public APIs as well:
> >>> StreamPartitioner#partition, to add the topic name string. Note it will
> >> be
> >>> a incompatible change that requires users who have customized
> >>> StreamPartitioner implementations.
> >>>
> >>> I've updated the wiki page of KIP-303, please recast your vote on this
> >>> thread. Thanks!!!
> >>>
> >>>
> >>> Guozhang
> >>>
> >>>
> >>> On Thu, May 17, 2018 at 3:15 PM, John Roesler <j...@confluent.io>
> wrote:
> >>>
> >>>> +1 non-binding
> >>>>
> >>>> On Thu, May 17, 2018 at 4:44 PM, Matthias J. Sax <
> matth...@confluent.io
> >>>
> >>>> wrote:
> >>>>
> >>>>> +1 (binding)
> >>>>>
> >>>>>
> >>>>> On 5/17/18 12:18 PM, Ted Yu wrote:
> >>>>>> +1
> >>>>>> -------- Original message --------From: Gwen Shapira <
> >>>> g...@confluent.io>
> >>>>> Date: 5/17/18 11:53 AM (GMT-08:00) To: dev <dev@kafka.apache.org>
> >>>>> Subject: Re: [VOTE] KIP-303: Add Dynamic Routing Support in Kafka
> >>>> Streams'
> >>>>> Topology Sink
> >>>>>> Yay, its about time :)
> >>>>>>
> >>>>>> +1
> >>>>>>
> >>>>>> On Thu, May 17, 2018 at 12:38 PM, Guozhang Wang <wangg...@gmail.com
> >
> >>>>> wrote:
> >>>>>>
> >>>>>>> Hello folks,
> >>>>>>>
> >>>>>>> I'd like to start a voting thread on adding dynamic routing
> >>>>> functionality
> >>>>>>> in Streams sink node. Please find a KIP here:
> >>>>>>>
> >>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>>> 303%3A+Add+Dynamic+Routing+in+Streams+Sink
> >>>>>>>
> >>>>>>>
> >>>>>>> And the PR itself ready for review as well under KAFKA-4936:
> >>>>>>>
> >>>>>>> https://github.com/apache/kafka/pull/5018
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> Thanks!
> >>>>>>> -- Guozhang
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >>
> >
> >
>
>
--
-- Guozhang
