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
