Hi, Artem, Thanks for the KIP. A few comments below.
1. I agree with Luke that having the partitioner returning -1 is kind of weird. Could we just change the implementation of DefaultPartitioner to the new behavior? 2. partitioner.sticky.batch.size: Similar question to Luke. I am not sure why we want to introduce this new configuration. Could we just use the existing batch.size? 4. I also agree with Luke that it's not clear why we need partition.availability.timeout.ms. The KIP says the broker "would not be chosen until the broker is able to accept the next ready batch from the partition". If we are keeping track of this, could we just avoid assigning records to partitions whose leader is not able to accept the next batch? If we do that, perhaps we don't need partition.availability.timeout.ms. 10. Currently, partitioner.class defaults to DefaultPartitioner, which uses StickyPartitioner when the key is specified. Since this KIP improves upon StickyPartitioner, it would be useful to make the new behavior the default and document that in the KIP. Thanks, Jun On Wed, Feb 16, 2022 at 7:30 PM Luke Chen <show...@gmail.com> wrote: > Hi Artem, > > Also, one more thing I think you need to know. > As this bug KAFKA-7572 <https://issues.apache.org/jira/browse/KAFKA-7572> > mentioned, sometimes the custom partitioner would return negative partition > id accidentally. > If it returned -1, how could you know if it is expected or not expected? > > Thanks. > Luke > > On Wed, Feb 16, 2022 at 3:28 PM Luke Chen <show...@gmail.com> wrote: > > > Hi Artem, > > > > Thanks for the update. I have some questions about it: > > > > 1. Could you explain why you need the `partitioner` return -1? In which > > case we need it? And how it is used in your KIP? > > 2. What does the "partitioner.sticky.batch.size" mean? In the > > "Configuration" part, you didn't explain it. And default to 0, I guess > it's > > the same as current behavior for backward compatibility, right? You > should > > mention it. > > 3. I'm thinking we can have a threshold to the > > "partitioner.sticky.batch.size". Let's say, we already accumulate 15.5KB > in > > partition1, and sent. So when next batch created, in your current design, > > we still stick to partition1, until 16KB reached, and then we create a > new > > batch to change to next partition, ex: partition2. But I think if we set > a > > threshold to 95% (for example), we can know previous 15.5KB already > exceeds > > the threshold so that we can directly create new batch for next records. > > This way should be able to make it more efficient. WDYT? > > 4. I think the improved queuing logic should be good enough. I can't get > > the benefit of having `partition.availability.timeout.ms` config. In > > short, you want to make the partitioner take the broker load into > > consideration. We can just improve that in the queuing logic (and you > > already did it). Why should we add the config? Could you use some > examples > > to explain why we need it. > > > > Thank you. > > Luke > > > > On Wed, Feb 16, 2022 at 8:57 AM Artem Livshits > > <alivsh...@confluent.io.invalid> wrote: > > > >> Hello, > >> > >> Please add your comments about the KIP. If there are no considerations, > >> I'll put it up for vote in the next few days. > >> > >> -Artem > >> > >> On Mon, Feb 7, 2022 at 6:01 PM Artem Livshits <alivsh...@confluent.io> > >> wrote: > >> > >> > Hello, > >> > > >> > After trying a few prototypes, I've made some changes to the public > >> > interface. Please see the updated document > >> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-794%3A+Strictly+Uniform+Sticky+Partitioner > >> > . > >> > > >> > -Artem > >> > > >> > On Thu, Nov 4, 2021 at 10:37 AM Artem Livshits < > alivsh...@confluent.io> > >> > wrote: > >> > > >> >> Hello, > >> >> > >> >> This is the discussion thread for > >> >> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-794%3A+Strictly+Uniform+Sticky+Partitioner > >> >> . > >> >> > >> >> The proposal is a bug fix for > >> >> https://issues.apache.org/jira/browse/KAFKA-10888, but it does > >> include a > >> >> client config change, therefore we have a KIP to discuss. > >> >> > >> >> -Artem > >> >> > >> > > >> > > >
