Moving the previous comment to the PR discussion. :) On Thu, Jun 27, 2019 at 10:51 AM Justine Olshan <jols...@confluent.io> wrote:
> 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 >> > > > >> > > >> > >> >
