Hey Everyone, Please consider this as a gentle reminder. Please have a look at the KIP and share your thoughts. https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937
On Tue, Jul 25, 2023 at 7:28 PM 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*
