Hi Matthias, Thanks for your review. The reason I chose to introduce a new abstract class is that, while it doesn't entail any changes in the StreamPartitioner interface, I also disabled the partition() method in that class. Reason to do that is that I didn't want a scenario where a user implements both partition and partitions methods which could lead to confusion. With the approach you suggested, while the interface still remains functional, users get the option to implement either methods which is what I wanted to avoid. Let me know if that makes sense.
Regarding extending StreamsPartitioner, we could expose net new to() methods taking in this new class devoid of any StreamPartitioner. I just thought it's cleaner to keep it this way as StreamPartitioner already dpes the partitioning. Let me know what you think. Thanks! Sagar. On Wed, Jun 29, 2022 at 5:34 AM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for the KIP. Overall a good addition. > > I am actually not sure if we need to add a new class? From my > understanding, if there is exactly one abstract method, the interface is > still functional? Thus, we could add a new method to > `StreamsPartitioner` with a default implementation (that calls the > existing `partition()` method and wraps the result into a singleton list)? > > Not sure what the pros/cons for both approaches would be? > > If we really add a new class, I am wondering if it should inherit from > `StreamsPartitioner` at all? Or course, if it does not, it's more stuff > we need to change, but the proposed overwrite of `partition()` that > throws also does not seem to be super clean? -- I am even wondering if > we should aim to deprecate the existing `partition()` and only offer > `partitions()` in the future? > > For the broadcast option, I am wondering if returning `null` (not an > singleton with `-1`) might be a clear option to encode it? Empty list > would still be valid as "send to no partition". > > Btw: The `StreamPartitioner` interface is also used for IQ. For both IQ > and FK-join, it seem ok to just add a runtime check that the returned > list is a singleton (in case we don't add a new class)? > > > -Matthias > > > On 6/26/22 7:55 AM, Sagar wrote: > > Hi Florin, > > > > Thanks for the comment! You brought up a very good point.. Actually I was > > focussed too much on the multicast operation and didn't pay attention to > > the other places where StramPartitioner is being used. TBH, I wasn't even > > aware that the StreamPartitioner is being used for FK joins so thanks > > definitely for pointing that out! > > > > Regarding how we handle that, I think since the FK join uses the > partition > > number info for subscription/message passing semantics, I would basically > > like to propose that we can throw an Exception when a user tries to pass > > an object which is an instance of MulticastPartitioner. This would keep > > things simple IMO because adding multicast keys to FK would just make it > > all the more complicated. > > > > Other than that, the usages/implementations of StreamPartitioner are on > > tests which would be taken care of if needed. > > Let me know what you think. > > > > Thanks! > > Sagar. > > > > > > On Sat, Jun 25, 2022 at 5:11 PM Florin Akermann < > florin.akerm...@gmail.com> > > wrote: > > > >> Hi Sagar, > >> > >> Thanks for the KIP. > >> > >> I am wondering about the following. What about other classes than the > >> org.apache.kafka.streams.processor.internals.RecordCollectorImpl. Would > >> they still call .partition(...) and just crash? Or is it a given that > they > >> never receive a reference to a Partitioner of > >> type MultiCastStreamPartitioner? > >> > >> Florin > >> > >> > >> On Sat, 28 May 2022 at 05:44, Sagar <sagarmeansoc...@gmail.com> wrote: > >> > >>> Hi All, > >>> > >>> I’m thinking to move this KIP to vote section if there aren’t any > >>> objections. > >>> > >>> Plz let me know if I that’s ok. > >>> > >>> Thanks! > >>> Sagar. > >>> > >>> On Tue, 24 May 2022 at 10:32 PM, Sagar <sagarmeansoc...@gmail.com> > >> wrote: > >>> > >>>> Hi All, > >>>> > >>>> Bumping this discussion thread again to see if there are any > >>>> comments/concerns on this. > >>>> > >>>> Thanks! > >>>> Sagar. > >>>> > >>>> On Wed, May 18, 2022 at 11:44 PM Sagar <sagarmeansoc...@gmail.com> > >>> wrote: > >>>> > >>>>> Hi All, > >>>>> > >>>>> I would like to open a discussion thread on > >>>>> > >>> > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211883356 > >>>>> . > >>>>> > >>>>> Thanks! > >>>>> Sagar. > >>>>> > >>>> > >>> > >> > > >
