Thanks Sagar -- one thing I'm still confused about, and sorry to keep pushing on this, but the example you gave for how this works in today's world seems not to correspond to the method described in the text of the Motivation exception, ie
Currently, if a user wants to replicate a message into N partitions, the > only way of doing that is to replicate the message N times and then plug-in > a custom partitioner to write the message N times into N different > partitions. This seems a little cumbersome way to broadcast. Also, there seems to be > no way of dropping a record within the partitioner. This KIP aims to make > this process simpler in Kafka Streams. It sounds like you're saying the problem this KIP is fixing is that the only way to do this is by implementing a custom partitioner and that this is cumbersome, but that's actually exactly what this KIP is doing: providing a method od multi-casting via implementing a custom partitioner (as seen in the example usage you provided). Thanks to your examples I think I now understand better what the KIP is doing, and assume what's written in the motivation section is just a type/mistake -- can you confirm? That said, the claim about having "no way of dropping a record within the partitioner" does actually seem to be correct, that is you couldn't do it with a custom partitioner prior to this KIP and now you can. I would consider that a secondary/additional improvement that these changes provide, but it's not strictly speaking related to multi-casting, right? (Just checking my understanding, not challenging anything about this) Cheers, Sophie On Thu, Aug 18, 2022 at 7:27 AM Sagar <sagarmeansoc...@gmail.com> wrote: > Hello Sophie, > > Thanks for your feedback. I have made all the suggested changes. > > One note, on how users can accomplish this in today's world , I have made > up this example and have never tried myself before. But I am assuming it > will work. > > Let me know what you think. > > Thanks! > Sagar. > > > On Thu, Aug 18, 2022 at 7:17 AM Sophie Blee-Goldman > <sop...@confluent.io.invalid> wrote: > > > 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. > > > > >> > > > > > > > > > >
