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
