All right, Thanks Andrew. Hey everyone, Please share your thoughts and feedback on the KIP : https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937
On Fri, Aug 4, 2023 at 2:50 AM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote: > Hi Jack, > I do understand the idea of extending the Partitioner interface so that > people are now able to use headers in the partitioning decision, and I see > that it’s filling in gap in the interface which was left when headers were > originally added. > > Experience with non-default partitioning schemes in the past makes me > unlikely to use anything other than the default partitioning scheme. > But I wouldn’t let that dissuade you. > > Thanks, > Andrew > > > On 3 Aug 2023, at 13:43, Jack Tomy <jacktomy...@gmail.com> wrote: > > > > Hey Andrew, Sagar > > > > Please share your thoughts. Thanks. > > > > > > > > On Mon, Jul 31, 2023 at 5:58 PM Jack Tomy <jacktomy...@gmail.com> wrote: > > > >> 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* > >> > > > > > > -- > > Best Regards > > *Jack* > > > -- Best Regards *Jack*
