Hello folks,
I'm closing this voting thread with 4 binding votes (Gwen, Matthias, Damian
and myself) and 3 non-binding votes (Ted, John, Bill).
Thanks for everyone for your feedbacks!
Guozhang
On Tue, May 22, 2018 at 8:13 AM, Guozhang Wang <wangg...@gmail.com> wrote:
> Hi Ismael,
>
> Yes, I have thought about deprecating the old API first, but I've decided
> to just go ahead and modify the public APIs because 1) with deprecated and
> new APIs, the internal implementation needs to do a runtime type check on
> each record, which is not efficient; 2) the affected public class is a less
> commonly used one, so the impact is expected to be small; 3) in 2.0.0 we
> are removing a list of deprecated APIs, plus we are introducing a few new
> APIs on the commonly used functions, so users likely need to make code
> changes even without this KIP anyways in their upgrade path.
>
>
> Guozhang
>
> On Tue, May 22, 2018 at 7:15 AM, Bill Bejeck <bbej...@gmail.com> wrote:
>
>> +1
>>
>>
>>
>> On Tue, May 22, 2018 at 3:46 AM, Ismael Juma <mli...@juma.me.uk> wrote:
>>
>> > Breaking API without a deprecation cycle doesn't seem good. Are we sure
>> > about this?
>> >
>> > Ismael
>> >
>> > On Mon, 21 May 2018, 15:04 Guozhang Wang, <wangg...@gmail.com> 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
>> > >
>> >
>>
>
>
>
> --
> -- Guozhang
>
--
-- Guozhang
