+1 On Tue, 22 May 2018 at 00:12, Gwen Shapira <g...@confluent.io> wrote:
> Recasting my +1 > > On Mon, May 21, 2018 at 3:38 PM, Guozhang Wang <wangg...@gmail.com> wrote: > > > 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 > > > > > > -- > *Gwen Shapira* > Product Manager | Confluent > 650.450.2760 | @gwenshap > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog > <http://www.confluent.io/blog> >