Hey Andrew, Sagar Thanks. I'm travelling so sorry for being brief and getting back late.
1. For the first concern, that is moving in a direction of server side partitioner, the idea seems very much promising but I believe we still have a long way to go. Since the proposal/design for the same is still not available, it's hard for me to defend my proposal. 2. For the second concern: 2.1 Loss of order in messages, I believe the ordering of messages is never promised and the partitioner has no requirement to ensure the same. It is upto the user to implement/use a partitioner which ensures ordering based on keys. 2.2 Key deciding the partitioner, It is totally up to the user to decide the partition regardless of the key, we are also passing the value to the partitioner. Even the existing implementation receives the value which lets the user decide the partition based on value. 2.3 Sending to a specific partition, for this, I need to be aware of the total number of partitions, but if I can do that same in partitioner, the cluster param gives me all the information I want. I would also quote a line from KIP-82 where headers were added to the serializer : The payload is traditionally for the business object, and *headers are traditionally used for transport routing*, filtering etc. So I believe when a user wants to add some routing information (in this case which set of partitions to go for), headers seem to be the right place. On Sat, Jul 29, 2023 at 8:48 PM Sagar <sagarmeansoc...@gmail.com> wrote: > Hi Andrew, > > Thanks for your comments. > > 1) Yes that makes sense and that's what even would expect to see as well. I > just wanted to highlight that we might still need a way to let client side > partitioning logic be present as well. Anyways, I am good on this point. > 2) The example provided does seem achievable by simply attaching the > partition number in the ProducerRecord. I guess if we can't find any > further examples which strengthen the case of this partitioner, it might be > harder to justify adding it. > > > Thanks! > Sagar. > > On Fri, Jul 28, 2023 at 2:05 PM Andrew Schofield < > andrew_schofield_j...@outlook.com> wrote: > > > Hi Sagar, > > Thanks for your comments. > > > > 1) Server-side partitioning doesn’t necessarily mean that there’s only > one > > way to do it. It just means that the partitioning logic runs on the > broker > > and > > any configuration of partitioning applies to the broker’s partitioner. If > > we ever > > see a KIP for this, that’s the kind of thing I would expect to see. > > > > 2) In the priority example in the KIP, there is a kind of contract > between > > the > > producers and consumers so that some records can be processed before > > others regardless of the order in which they were sent. The producer > > wants to apply special significance to a particular header to control > which > > partition is used. I would simply achieve this by setting the partition > > number > > in the ProducerRecord at the time of sending. > > > > I don’t think the KIP proposes adjusting the built-in partitioner or > > adding to AK > > a new one that uses headers in the partitioning decision. So, any > > configuration > > for a partitioner that does support headers would be up to the > > implementation > > of that specific partitioner. Partitioner implements Configurable. > > > > I’m just providing an alternative view and I’m not particularly opposed > to > > the KIP. > > I just don’t think it quite merits the work involved to get it voted and > > merged. > > As an aside, a long time ago, I created a small KIP that was never > adopted > > and I didn’t push it because I eventually didn’t need it. > > > > Thanks, > > Andrew > > > > > On 28 Jul 2023, at 05:15, Sagar <sagarmeansoc...@gmail.com> wrote: > > > > > > Hey Andrew, > > > > > > Thanks for the review. Since I had reviewed the KIP I thought I would > > also > > > respond. Of course Jack has the final say on this since he wrote the > KIP. > > > > > > 1) This is an interesting point and I hadn't considered it. The > > > comparison with KIP-848 is a valid one but even within that KIP, it > > allows > > > client side partitioning for power users like Streams. So while we > would > > > want to move away from client side partitioner as much as possible, we > > > still shouldn't do away completely with Client side partitioning and > end > > up > > > being in a state of inflexibility for different kinds of usecases. This > > is > > > my opinion though and you have more context on Clients, so would like > to > > > know your thoughts on this. > > > > > > 2) Regarding this, I assumed that since the headers are already part of > > the > > > consumer records they should have access to the headers and if there > is a > > > contract b/w the applications producing and the application consuming, > > that > > > decisioning should be transparent. Was my assumption incorrect? But as > > you > > > rightly pointed out header based partitioning with keys is going to > lead > > to > > > surprising results. Assuming there is merit in this proposal, do you > > think > > > we should ignore the keys in this case (similar to the effect of > > > setting *partitioner.ignore.keys > > > *config to false) and document it appropriately? > > > > > > Let me know what you think. > > > > > > Thanks! > > > Sagar. > > > > > > > > > On Thu, Jul 27, 2023 at 9:41 PM Andrew Schofield < > > > andrew_schofield_j...@outlook.com> wrote: > > > > > >> Hi Jack, > > >> Thanks for the KIP. I have a few concerns about the idea. > > >> > > >> 1) I think that while a client-side partitioner seems like a neat idea > > and > > >> it’s an established part of Kafka, > > >> it’s one of the things which makes Kafka clients quite complicated. > Just > > >> as KIP-848 is moving from > > >> client-side assignors to server-side assignors, I wonder whether > really > > we > > >> should be looking to make > > >> partitioning a server-side capability too over time. So, I’m not > > convinced > > >> that making the Partitioner > > >> interface richer is moving in the right direction. > > >> > > >> 2) For records with a key, the partitioner usually calculates the > > >> partition from the key. This means > > >> that records with the same key end up on the same partition. Many > > >> applications expect this to give ordering. > > >> Log compaction expects this. There are situations in which records > have > > to > > >> be repartitioned, such as > > >> sometimes happens with Kafka Streams. I think that a header-based > > >> partitioner for records which have > > >> keys is going to be surprising and only going to have limited > > >> applicability as a result. > > >> > > >> The tricky part about clever partitioning is that downstream systems > > have > > >> no idea how the partition > > >> number was arrived at, so they do not truly understand how the > ordering > > >> was derived. I do think that > > >> perhaps there’s value to being able to influence the partitioning > using > > >> the headers, but I wonder if actually > > >> transforming the headers into an “ordering context” that then flows > with > > >> the record as it moves through > > >> the system would be a stronger solution. Today, the key is the > ordering > > >> context. Maybe it should be a > > >> concept in its own right and the Producer could configure a converter > > from > > >> headers to ordering context. > > >> That is of course a much bigger change. > > >> > > >> In one of the examples you mention in the KIP, you mentioned using a > > >> header to control priority. I guess the > > >> idea is to preferentially process records off specific partitions so > > they > > >> can overtake lower priority records. > > >> I suggest just sending the records explicitly to specific partitions > to > > >> achieve this. > > >> > > >> Sorry for the essay, but you did ask for people to share their > thoughts > > :) > > >> > > >> Just my opinion. Let’s see what others think. > > >> > > >> Thanks, > > >> Andrew > > >> > > >>> On 25 Jul 2023, at 14:58, Jack Tomy <jacktomy...@gmail.com> wrote: > > >>> > > >>> Hey @Sagar > > >>> > > >>> Thanks again for the review. > > >>> 1. "a null headers value is equivalent to invoking the older > partition > > >>> method", this is not true. If someone makes an implementation and the > > >>> headers come as null, still the new implementation will take effect. > > >>> Instead I have added : "Not overriding this method in the Partitioner > > >>> interface has the same behaviour as using the existing method." > > >>> 2. Corrected. > > >>> > > >>> Hey @Sagar and everyone, > > >>> Please have a look at the new version and share your thoughts. > > >>> > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937 > > >>> Like Sagar mentioned, I would also request more people who have more > > >>> context on clients to chime in. > > >>> > > >>> > > >>> On Tue, Jul 25, 2023 at 2:58 PM Sagar <sagarmeansoc...@gmail.com> > > wrote: > > >>> > > >>>> Hi Jack, > > >>>> > > >>>> Thanks I have a couple of final comments and then I am good. > > >>>> > > >>>> 1) Can you elaborate on the Javadocs of the partition headers > argument > > >> to > > >>>> specify that a null headers value is equivalent to invoking the > older > > >>>> partition method? It is apparent but generally good to call out. > > >>>> 2) In the Compatibility section, you have mentioned backward > > >> comparable. I > > >>>> believe it should be *backward compatible change.* > > >>>> > > >>>> I don't have other comments. Post this, probably someone else who > has > > >> more > > >>>> context on Clients can also chime in on this before we can move this > > to > > >>>> Voting. > > >>>> > > >>>> Thanks! > > >>>> Sagar. > > >>>> > > >>>> > > >>>> On Sat, Jul 22, 2023 at 10:09 AM Jack Tomy <jacktomy...@gmail.com> > > >> wrote: > > >>>> > > >>>>> Hey @Sagar, > > >>>>> > > >>>>> Thank you again for the response and feedback. > > >>>>> > > >>>>> 1. Though the ask wasn't very clear to me I have attached the > > Javadoc > > >>>> as > > >>>>> per your suggestion. Please have a look and let me know if this > > meets > > >>>>> the > > >>>>> expectations. > > >>>>> 2. Done. > > >>>>> 3. Done > > >>>>> 4. Done > > >>>>> > > >>>>> Hey @Sagar and everyone, > > >>>>> Please have a look at the new version and share your thoughts. > > >>>>> > > >>>> > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937 > > >>>>> > > >>>>> On Thu, Jul 20, 2023 at 9:46 PM Sagar <sagarmeansoc...@gmail.com> > > >> wrote: > > >>>>> > > >>>>>> Thanks Jack for the updates. > > >>>>>> > > >>>>>> Some more feedback: > > >>>>>> > > >>>>>> 1) It would be better if you can add the Javadoc in the Public > > >>>> interfaces > > >>>>>> section. That is a general practice used which gives the readers > of > > >> the > > >>>>> KIP > > >>>>>> a high level idea of the Public Interfaces. > > >>>>>> > > >>>>>> 2) In the proposed section, the bit about marking headers as read > > only > > >>>>>> seems like an implementation detail This can generally be avoided > in > > >>>>> KIPs. > > >>>>>> > > >>>>>> 3) Also, in the Deprecation section, can you mention again that > this > > >>>> is a > > >>>>>> backward compatible change and the reason for it (already done in > > the > > >>>>>> Proposed Changes section). > > >>>>>> > > >>>>>> 4) In the Testing Plan section, there is still the KIP template > bit > > >>>>> copied > > >>>>>> over. That can be removed. > > >>>>>> > > >>>>>> Thanks! > > >>>>>> Sagar. > > >>>>>> > > >>>>>> > > >>>>>> On Thu, Jul 20, 2023 at 2:48 PM Jack Tomy <jacktomy...@gmail.com> > > >>>> wrote: > > >>>>>> > > >>>>>>> Hey Everyone, > > >>>>>>> > > >>>>>>> Please consider this as a reminder and share your feedback. Thank > > >>>> you. > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937 > > >>>>>>> > > >>>>>>> On Tue, Jul 18, 2023 at 5:43 PM Jack Tomy <jacktomy...@gmail.com > > > > >>>>> wrote: > > >>>>>>> > > >>>>>>>> Hey @Sagar, > > >>>>>>>> > > >>>>>>>> Thank you for the response and feedback. > > >>>>>>>> > > >>>>>>>> 1. Done > > >>>>>>>> 2. Yeah, that was a mistake from my end. Corrected. > > >>>>>>>> 3. Can you please elaborate this, I have added the java doc > > >>>> along > > >>>>>> with > > >>>>>>>> the code changes. Should I paste the same in KIP too? > > >>>>>>>> 4. Moved. > > >>>>>>>> 5. I have added one more use case, it is actually helpful in > any > > >>>>>>>> situation where you want to pass some information to partition > > >>>>>> method > > >>>>>>> but > > >>>>>>>> don't have to have it in the key or value. > > >>>>>>>> 6. Added. > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> Hey @Sagar and everyone, > > >>>>>>>> Please have a look at the new version and share your thoughts. > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937 > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> On Tue, Jul 18, 2023 at 9:53 AM Sagar < > sagarmeansoc...@gmail.com> > > >>>>>> wrote: > > >>>>>>>> > > >>>>>>>>> Hi Jack, > > >>>>>>>>> > > >>>>>>>>> Thanks for the KIP! Seems like an interesting idea. I have some > > >>>>>>> feedback: > > >>>>>>>>> > > >>>>>>>>> 1) It would be great if you could clean up the text that seems > to > > >>>>>> mimic > > >>>>>>>>> the > > >>>>>>>>> KIP template. It is generally not required in the KIP. > > >>>>>>>>> > > >>>>>>>>> 2) In the Public Interfaces where you mentioned *Partitioner > > >>>> method > > >>>>> in > > >>>>>>>>> **org/apache/kafka/clients/producer > > >>>>>>>>> will have the following update*, I believe you meant the > > >>>> Partitioner > > >>>>>>>>> *interface*? > > >>>>>>>>> > > >>>>>>>>> 3) Staying on Public Interface, it is generally preferable to > add > > >>>> a > > >>>>>>>>> Javadocs section along with the newly added method. You could > > also > > >>>>>>>>> describe > > >>>>>>>>> the behaviour of it invoking the default existing method. > > >>>>>>>>> > > >>>>>>>>> 4) The option that is mentioned in the Rejected Alternatives, > > >>>> seems > > >>>>>> more > > >>>>>>>>> like a workaround to the current problem that you are > describing. > > >>>>> That > > >>>>>>>>> could be added to the Motivation section IMO. > > >>>>>>>>> > > >>>>>>>>> 5) Can you also add some more examples of scenarios where this > > >>>> would > > >>>>>> be > > >>>>>>>>> helpful? The only scenario mentioned seems to have a > workaround. > > >>>>> Just > > >>>>>>>>> trying to ensure that we have a strong enough motivation before > > >>>>>> adding a > > >>>>>>>>> public API. > > >>>>>>>>> > > >>>>>>>>> 6) One thing which should also be worth noting down would be > what > > >>>>>>> happens > > >>>>>>>>> if users override both methods, only one method (new or old) > and > > >>>> no > > >>>>>>>>> methods > > >>>>>>>>> (the default behaviour). It would help in understanding the > > >>>> proposal > > >>>>>>>>> better. > > >>>>>>>>> > > >>>>>>>>> Thanks! > > >>>>>>>>> Sagar. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> On Mon, Jul 17, 2023 at 9:19 PM Jack Tomy < > jacktomy...@gmail.com > > > > > >>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>>> Hey everyone, > > >>>>>>>>>> > > >>>>>>>>>> Not seeing much discussion on the KPI. Might be because it is > > >>>> too > > >>>>>>>>>> obvious 😉. > > >>>>>>>>>> > > >>>>>>>>>> If there are no more comments, I will start the VOTE in the > > >>>> coming > > >>>>>>> days. > > >>>>>>>>>> > > >>>>>>>>>> On Sat, Jul 15, 2023 at 8:48 PM Jack Tomy < > > >>>> jacktomy...@gmail.com> > > >>>>>>>>> wrote: > > >>>>>>>>>> > > >>>>>>>>>>> Hey everyone, > > >>>>>>>>>>> > > >>>>>>>>>>> Please take a look at the KPI below and provide your > > >>>> suggestions > > >>>>>> and > > >>>>>>>>>>> feedback. TIA. > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937 > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> -- > > >>>>>>>>>>> Best Regards > > >>>>>>>>>>> *Jack* > > >>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> -- > > >>>>>>>>>> Best Regards > > >>>>>>>>>> *Jack* > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> -- > > >>>>>>>> Best Regards > > >>>>>>>> *Jack* > > >>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> -- > > >>>>>>> Best Regards > > >>>>>>> *Jack* > > >>>>>>> > > >>>>>> > > >>>>> > > >>>>> > > >>>>> -- > > >>>>> Best Regards > > >>>>> *Jack* > > >>>>> > > >>>> > > >>> > > >>> > > >>> -- > > >>> Best Regards > > >>> *Jack* > > >> > > >> > > > > > -- Best Regards *Jack*
