Thanks for updating! I looked over the changes and I think it's good. Walker
On Tue, Aug 9, 2022 at 5:44 AM Sagar <sagarmeansoc...@gmail.com> wrote: > 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. > >> > > >>>>>>> > >> > > >>>>>> > >> > > >>>>> > >> > > >>>> > >> > > >>> > >> > > >> > >> > > > > >> > > > >> > > >> > > >
