Stanislav,
Thank you for looking at my KIP!
I did discuss with Colin about whether the null vs. Optional types and we
did not come to a strong conclusion either way.
I'd be happy to change it if it makes the logic more clear.
Thanks,
Justine
On Wed, Jun 26, 2019 at 2:46 PM Stanislav Kozlovski <stanis...@confluent.io>
wrote:
> Hey Justine,
>
> Thanks for the KIP! I am impressed by the performance results linked in the
> KIP and I like the data-driven approach. This looks like a great
> improvement.
>
> I had one minor question regarding the public interface
> `repartitionOnNewBatch` where we return null in the case of no change
> needed. Have we considered using Java's Optional type to avoid null values?
>
> Best,
> Stanislav
>
> On Tue, Jun 25, 2019 at 11:29 PM Colin McCabe <cmcc...@apache.org> wrote:
>
> > No worries. Thanks for fixing it!
> > C.
> >
> > On Tue, Jun 25, 2019, at 13:47, Justine Olshan wrote:
> > > Also apologies on the late link to the jira, but apparently https links
> > do
> > > not work and it kept defaulting to an image on my desktop even when it
> > > looked like I put the correct link in. Weird...
> > >
> > > On Tue, Jun 25, 2019 at 1:41 PM Justine Olshan <jols...@confluent.io>
> > wrote:
> > >
> > > > I came up with a good solution for this and will push the commit
> soon.
> > The
> > > > repartition will be called only when a partition is not manually
> sent.
> > > >
> > > > On Tue, Jun 25, 2019 at 1:39 PM Colin McCabe <cmcc...@apache.org>
> > wrote:
> > > >
> > > >> Well, this is a generic partitioner method, so it shouldn't dictate
> > any
> > > >> particular behavior.
> > > >>
> > > >> Colin
> > > >>
> > > >>
> > > >> On Tue, Jun 25, 2019, at 12:04, Justine Olshan wrote:
> > > >> > I also just noticed that if we want to use this method on the
> keyed
> > > >> record
> > > >> > case, I will need to move the method outside of the sticky (no
> key,
> > no
> > > >> set
> > > >> > partition) check. Not a big problem, but something to keep in
> mind.
> > > >> > Perhaps, we should encapsulate the sticky vs. not behavior inside
> > the
> > > >> > method? More things to think about.
> > > >> >
> > > >> > On Tue, Jun 25, 2019 at 11:55 AM Colin McCabe <cmcc...@apache.org
> >
> > > >> wrote:
> > > >> >
> > > >> > > Hi Justine,
> > > >> > >
> > > >> > > The KIP discusses adding a new method to the partitioner
> > interface.
> > > >> > >
> > > >> > > > default public Integer onNewBatch(String topic, Cluster
> > cluster) {
> > > >> ... }
> > > >> > >
> > > >> > > However, this new method doesn't give the partitioner access to
> > the
> > > >> key
> > > >> > > and value of the message. While this works for the case
> described
> > > >> here (no
> > > >> > > key), in general we might need this information when
> re-assigning
> > a
> > > >> > > partitition based on the batch completing. So I think we should
> > add
> > > >> these
> > > >> > > methods to onNewBatch.
> > > >> > >
> > > >> > > Also, it would be nice to call this something like
> > > >> "repartitionOnNewBatch"
> > > >> > > or something, to make it clearer what is going on.
> > > >> > >
> > > >> > > best,
> > > >> > > Colin
> > > >> > >
> > > >> > > On Mon, Jun 24, 2019, at 18:32, Boyang Chen wrote:
> > > >> > > > Thank you Justine for the KIP! Do you mind creating a
> > corresponding
> > > >> JIRA
> > > >> > > > ticket too?
> > > >> > > >
> > > >> > > > On Mon, Jun 24, 2019 at 4:51 PM Colin McCabe <
> > cmcc...@apache.org>
> > > >> wrote:
> > > >> > > >
> > > >> > > > > Hi Justine,
> > > >> > > > >
> > > >> > > > > Thanks for the KIP. This looks great!
> > > >> > > > >
> > > >> > > > > In one place in the KIP, you write: "Remove
> > > >> > > > > testRoundRobinWithUnavailablePartitions() and
> testRoundRobin()
> > > >> since
> > > >> > > the
> > > >> > > > > round robin functionality of the partitioner has been
> > removed."
> > > >> You
> > > >> > > can
> > > >> > > > > skip this and similar lines. We don't need to describe
> > changes to
> > > >> > > internal
> > > >> > > > > test classes in the KIP since they're not visible to users
> or
> > > >> external
> > > >> > > > > developers.
> > > >> > > > >
> > > >> > > > > It seems like maybe the performance tests should get their
> own
> > > >> section.
> > > >> > > > > Right now, the way the layout is makes it look like they are
> > part
> > > >> of
> > > >> > > the
> > > >> > > > > "Compatibility, Deprecation, and Migration Plan"
> > > >> > > > >
> > > >> > > > > best,
> > > >> > > > > Colin
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > On Mon, Jun 24, 2019, at 14:04, Justine Olshan wrote:
> > > >> > > > > > Hello,
> > > >> > > > > > This is the discussion thread for KIP-480: Sticky
> > Partitioner.
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > >
> > > >> > >
> > > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-480%3A+Sticky+Partitioner
> > > >> > > > > >
> > > >> > > > > > Thank you,
> > > >> > > > > > Justine Olshan
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>
>
> --
> Best,
> Stanislav
>
