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* >> >>
