Hello All, Bumping this one again to see if folks have any other comments/observations.
Thanks! Sagar. On Wed, Jul 27, 2022 at 4:03 PM Sagar <sagarmeansoc...@gmail.com> wrote: > 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. >> > > >>>>>>> >> > > >>>>>> >> > > >>>>> >> > > >>>> >> > > >>> >> > > >> >> > > > >> > > >> > >> >
