Thanks Walker for the comments I have updated the KIP with all the suggestions.
Thanks! On Tue, Jul 12, 2022 at 10:59 PM Walker Carlson <wcarl...@confluent.io.invalid> wrote: > Hi Sagar, > > I just finished reading the KIP and this seems to be a great addition. > > I agree with Matthias that the interface with a default implementation and > deprecating partition() does seem cleaner. It has been a pattern that we > have followed in the past. How I would handle a custom streams partitioner > is just always call partitions(). If it is implemented then we ignore the > partition() and if not the default implementation should just wrap the > deprecated method in a list. > > Despite that I think your concerns are valid about this causing some > confusion. To avoid that in the past we have just made sure we updated the > depreciation message very cleanly and also include that implementing the > new method will override the old one in the description. All those docs > plus good logging has worked well. We had a very similar situation when > adding a new exception handler for streams back for 2.8 and these > precautions seemed to be enough. > > thanks for the kip! > Walker > > On Sun, Jul 10, 2022 at 1:22 PM Sagar <sagarmeansoc...@gmail.com> wrote: > > > 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. > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > > > > >
