Hi Sagar,
Thank you for the KIP and sorry for being late to the party!
1.
The java docs for partitions() say:
"Note that returning a single valued list with value -1 is a shorthand
for broadcasting the record to all the partitions of the topic."
I guess that is not true anymore since the code in the "Proposed
Changes" section checks for null to decide about broadcasting.
2.
I would prefer changing the return type of partitions() to
Optional<List<Integer>> and using Optional.empty() as the broadcast
value. IMO, The chances that an implementation returns null due to a bug
is much higher than that an implementation returns an empty Optional due
to a bug. I would also be fine with a singleton list with a -1 as you
describe in the java docs.
3.
Recently a "Test Plan" section was added to the KIP template. Your KIP
is missing this section.
Best,
Bruno
On 24.08.22 00:22, Sophie Blee-Goldman wrote:
Thanks for the updates, it reads much more clearly to me now. Looks great
+1 (binding)
Cheers,
Sophie
On Fri, Aug 19, 2022 at 1:42 AM Sagar <sagarmeansoc...@gmail.com> wrote:
Thanks Sophie for the review. I see the confusion. As you pointed out, the
problem the KIP is trying to solve is not the avoidance of a custom
partitioner. Instead the process of sending or replicating the message N
times and then having the record wired through via a new custom partitioner
for every replication. That's what I tried to convey in the motivation
section. I updated the motivation slightly, let me know if that sounds ok.
Also, yes the dropping of records using a custom partitioner is an added
benefit that we get. I think the custom partitioner bit is important as one
can always filter the records out initially.
Let me know if this looks ok?
Thanks!
Sagar.
On Fri, Aug 19, 2022 at 10:17 AM Sophie Blee-Goldman
<sop...@confluent.io.invalid> wrote:
Thanks Sagar -- one thing I'm still confused about, and sorry to keep
pushing on this, but the example
you gave for how this works in today's world seems not to correspond to
the
method described in the
text of the Motivation exception, ie
Currently, if a user wants to replicate a message into N partitions, the
only way of doing that is to replicate the message N times and then
plug-in
a custom partitioner to write the message N times into N different
partitions.
This seems a little cumbersome way to broadcast. Also, there seems to be
no way of dropping a record within the partitioner. This KIP aims to
make
this process simpler in Kafka Streams.
It sounds like you're saying the problem this KIP is fixing is that the
only way to do this is by implementing
a custom partitioner and that this is cumbersome, but that's actually
exactly what this KIP is doing: providing
a method od multi-casting via implementing a custom partitioner (as seen
in
the example usage you provided).
Thanks to your examples I think I now understand better what the KIP is
doing, and assume what's written in
the motivation section is just a type/mistake -- can you confirm?
That said, the claim about having "no way of dropping a record within the
partitioner" does actually seem to be
correct, that is you couldn't do it with a custom partitioner prior to
this
KIP and now you can. I would consider
that a secondary/additional improvement that these changes provide, but
it's not strictly speaking related to
multi-casting, right? (Just checking my understanding, not challenging
anything about this)
Cheers,
Sophie
On Thu, Aug 18, 2022 at 7:27 AM Sagar <sagarmeansoc...@gmail.com> wrote:
Hello Sophie,
Thanks for your feedback. I have made all the suggested changes.
One note, on how users can accomplish this in today's world , I have
made
up this example and have never tried myself before. But I am assuming
it
will work.
Let me know what you think.
Thanks!
Sagar.
On Thu, Aug 18, 2022 at 7:17 AM Sophie Blee-Goldman
<sop...@confluent.io.invalid> wrote:
Hey Sagar, thanks for the KIP!
Just some cosmetic points to make it absolutely clear what this KIP
is
doing:
1) could you clarify up front in the Motivation section that this is
focused on Kafka Streams applications, and not the plain Producer
client?
2) you included the entire implementation of the `#send` method to
demonstrate the change in logic, but can you either remove the parts
of
the implementation that aren't being touched here or at least
highlight
in
some way the specific lines that have changed?
3) In general the implementation is, well, an implementation detail
that
doesn't need to be included in the KIP, but it's ok -- always nice to
get a
sense of how things will work internally. But what I think would be
more
useful to show in the KIP is how things will work with the new public
interface -- ie, can you provide a brief example of how a user would
go
about taking advantage of this new interface? Even better, include an
example of what it takes for a user to accomplish this behavior
before
this
KIP. It would help showcase the concrete benefit this KIP is bringing
and
anchor the motivation section a bit better.
Also nit: in the 2nd sentence under Public Interfaces I think it
should
say
"it would invoke the partition() method" -- ie this should be
"partition()" not "partition*s*()"
Other than that this looks good, but I'll wait until you've addressed
the
above to cast a vote.
Thanks!
Sophie
On Fri, Aug 12, 2022 at 10:36 PM Sagar <sagarmeansoc...@gmail.com>
wrote:
Hey John,
Thanks for the vote. I added the reason for the rejection of the
alternatives. The first one is basically an option to broadcast to
all
partitions which I felt was restrictive. Instead the KIP allows
multicasting to 0-N partitions based upon the partitioner
implementation.
Thanks!
Sagar.
On Sat, Aug 13, 2022 at 7:35 AM John Roesler <vvcep...@apache.org>
wrote:
Thanks, Sagar!
I’m +1 (binding)
Can you add a short explanation to each rejected alternative? I
was
wondering why we wouldn’t provide an overloaded to()/addSink()
(the
first
rejected alternative), and I had to look back at the Streams code
to
see
that they both already accept the partitioner (I thought it was a
config).
Thanks!
-John
On Tue, Aug 9, 2022, at 13:44, Walker Carlson wrote:
+1 (non binding)
Walker
On Tue, May 31, 2022 at 4:44 AM Sagar <
sagarmeansoc...@gmail.com
wrote:
Hi All,
I would like to start a voting thread on
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211883356
.
I am just starting this as the discussion thread has been open
for
10+
days. In case there are some comments, we can always discuss
them
over
there.
Thanks!
Sagar.
