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
