Thanks for the updates, it reads much more clearly to me now. Looks great +1 (binding)
Cheers, Sophie On Fri, Aug 19, 2022 at 1:42 AM Sagar <sagarmeansoc...@gmail.com> wrote: > 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. > > > > > > >> > > > > > > > > > > > > > > > > > > > > >
