You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Florin Akermann <fl...@gmail.com> on 2022/06/25 11:41:03 UTC
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
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 <sa...@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 <sa...@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 <sa...@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.
> >>
> >
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Walker Carlson <wc...@confluent.io.INVALID>.
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 <sa...@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 <sa...@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
> > <wc...@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 <sa...@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 <mjsax@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.akermann@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 <sagarmeansocean@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 <
> >> sagarmeansocean@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 <
> >> sagarmeansocean@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.
> >> > > >>>>>>>
> >> > > >>>>>>
> >> > > >>>>>
> >> > > >>>>
> >> > > >>>
> >> > > >>
> >> > > >
> >> > >
> >> >
> >>
> >
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Sagar <sa...@gmail.com>.
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 <sa...@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
> <wc...@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 <sa...@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.akermann@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 <sa...@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 <
>> sagarmeansocean@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 <
>> sagarmeansocean@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.
>> > > >>>>>>>
>> > > >>>>>>
>> > > >>>>>
>> > > >>>>
>> > > >>>
>> > > >>
>> > > >
>> > >
>> >
>>
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Sagar <sa...@gmail.com>.
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
<wc...@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 <sa...@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.akermann@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 <sa...@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 <
> sagarmeansocean@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 <
> sagarmeansocean@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.
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Walker Carlson <wc...@confluent.io.INVALID>.
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 <sa...@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.akermann@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 <sa...@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 <sa...@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 <sagarmeansocean@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.
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> >
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Sagar <sa...@gmail.com>.
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.akermann@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 <sa...@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 <sa...@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 <sa...@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.
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by "Matthias J. Sax" <mj...@apache.org>.
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.akermann@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 <sa...@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 <sa...@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 <sa...@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.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Sagar <sa...@gmail.com>.
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.akermann@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 <sa...@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 <sa...@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 <sa...@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.
> >>>>>
> >>>>
> >>>
> >>
> >
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by "Matthias J. Sax" <mj...@apache.org>.
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 <fl...@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 <sa...@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 <sa...@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 <sa...@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.
>>>>>
>>>>
>>>
>>
>
Re: [DISCUSS] KIP-837 Allow MultiCasting a Result Record.
Posted by Sagar <sa...@gmail.com>.
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 <fl...@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 <sa...@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 <sa...@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 <sa...@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.
> > >>
> > >
> >
>