Hey Sagar, thanks for the KIP! Just some cosmetic points to make it absolutely clear what this KIP is doing: 1) could you clarify up front in the Motivation section that this is focused on Kafka Streams applications, and not the plain Producer client? 2) you included the entire implementation of the `#send` method to demonstrate the change in logic, but can you either remove the parts of the implementation that aren't being touched here or at least highlight in some way the specific lines that have changed? 3) In general the implementation is, well, an implementation detail that doesn't need to be included in the KIP, but it's ok -- always nice to get a sense of how things will work internally. But what I think would be more useful to show in the KIP is how things will work with the new public interface -- ie, can you provide a brief example of how a user would go about taking advantage of this new interface? Even better, include an example of what it takes for a user to accomplish this behavior before this KIP. It would help showcase the concrete benefit this KIP is bringing and anchor the motivation section a bit better.
Also nit: in the 2nd sentence under Public Interfaces I think it should say "it would invoke the partition() method" -- ie this should be "partition()" not "partition*s*()" Other than that this looks good, but I'll wait until you've addressed the above to cast a vote. Thanks! Sophie On Fri, Aug 12, 2022 at 10:36 PM Sagar <sagarmeansoc...@gmail.com> wrote: > Hey John, > > Thanks for the vote. I added the reason for the rejection of the > alternatives. The first one is basically an option to broadcast to all > partitions which I felt was restrictive. Instead the KIP allows > multicasting to 0-N partitions based upon the partitioner implementation. > > Thanks! > Sagar. > > On Sat, Aug 13, 2022 at 7:35 AM John Roesler <vvcep...@apache.org> wrote: > > > Thanks, Sagar! > > > > I’m +1 (binding) > > > > Can you add a short explanation to each rejected alternative? I was > > wondering why we wouldn’t provide an overloaded to()/addSink() (the first > > rejected alternative), and I had to look back at the Streams code to see > > that they both already accept the partitioner (I thought it was a > config). > > > > Thanks! > > -John > > > > On Tue, Aug 9, 2022, at 13:44, Walker Carlson wrote: > > > +1 (non binding) > > > > > > Walker > > > > > > On Tue, May 31, 2022 at 4:44 AM Sagar <sagarmeansoc...@gmail.com> > wrote: > > > > > >> Hi All, > > >> > > >> I would like to start a voting thread on > > >> > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211883356 > > >> . > > >> > > >> I am just starting this as the discussion thread has been open for 10+ > > >> days. In case there are some comments, we can always discuss them over > > >> there. > > >> > > >> Thanks! > > >> Sagar. > > >> > > >
