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