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.
