Thanks Sophie for the review. I see the confusion. As you pointed out, the problem the KIP is trying to solve is not the avoidance of a custom partitioner. Instead the process of sending or replicating the message N times and then having the record wired through via a new custom partitioner for every replication. That's what I tried to convey in the motivation section. I updated the motivation slightly, let me know if that sounds ok.
Also, yes the dropping of records using a custom partitioner is an added benefit that we get. I think the custom partitioner bit is important as one can always filter the records out initially. Let me know if this looks ok? Thanks! Sagar. On Fri, Aug 19, 2022 at 10:17 AM Sophie Blee-Goldman <sop...@confluent.io.invalid> wrote: > 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. > > > > > >> > > > > > > > > > > > > > > >
