I was going through fixing some of the overloaded methods and I realized I overloaded the RecordAccumulator constructor. I added a parameter to include the partitioner so I can call my method. However, the tests for the record accumulator do not have a partitioner. There is the potential for a NPE when calling this method. Currently, none of the tests will enter the code block, but I was wondering if would be a good idea to include a partitioner != null in the if statement as well. I'm open to other suggestions if this is not clear about what is going on.
Ismael, Oh I see now. It seems like Netflix just checks if the leader is available as well. I'll look into the case where no replica is down. On Thu, Jun 27, 2019 at 10:39 AM Ismael Juma <isma...@gmail.com> wrote: > Hey Justine. > > Available could mean that some replicas are down but the leader is > available. The suggestion was to try a partition where no replica was down > if it's available. Such partitions are safer in general. There could be > some downsides too, so worth thinking about the trade-offs. > > Ismael > > On Thu, Jun 27, 2019, 10:24 AM Justine Olshan <jols...@confluent.io> > wrote: > > > Ismael, > > > > Thanks for the feedback! > > > > For 1, currently the sticky partitioner favors "available partitions." > From > > my understanding, these are partitions that are not under-replicated. If > > that is not the same, please let me know. > > As for 2, I've switched to Optional, and the few tests I've run so far > > suggest the performance is the same. > > And for 3, I've added a javadoc to my next commit, so that should be up > > soon. > > > > Thanks, > > Justine > > > > On Thu, Jun 27, 2019 at 1:31 AM Ismael Juma <ism...@juma.me.uk> wrote: > > > > > Thanks for the KIP Justine. It looks pretty good. A few comments: > > > > > > 1. Should we favor partitions that are not under replicated? This is > > > something that Netflix did too. > > > > > > 2. If there's no measurable performance difference, I agree with > > Stanislav > > > that Optional would be better than Integer. > > > > > > 3. We should include the javadoc for the newly introduced method that > > > specifies it and its parameters. In particular, it would good to > specify > > if > > > it gets called when an explicit partition id has been provided. > > > > > > Ismael > > > > > > On Mon, Jun 24, 2019, 2:04 PM Justine Olshan <jols...@confluent.io> > > 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 > > > > > > > > > >
