Hi Matthias, I agree that working with interfaces is cleaner. As such, there's not much value in keeping both the methods. So, we can go down the route of deprecating partition(). The only question I have is till deprecation if we get both partition() and partitions() implemented, we may need to give precedence to partitions() method, right?
Also, in IQ and FK-join the runtime check you proposed seems good to me and your suggestion on broadcast makes sense as well. Lastly, I am leaning towards the interface approach now. Let's see if other have any questions/comments. Thanks! Sagar. On Fri, Jul 8, 2022 at 4:31 AM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for explaining you reasoning. > > I agree that it might not be ideal to have both methods implemented, but > if we deprecate the exiting one, it would only be an issue until we > remove the old one? Or do we see value to keep both methods? > > In general, working with interfaces is cleaner than with abstract > classed, that is why I proposed it. > > In the end, I don't have too strong of an opinion what the better option > would be. Maybe others can chime in and share their thoughts? > > -Matthias > > On 7/1/22 10:54 PM, Sagar wrote: > > 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. > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
