Thanks.
Glad we could align.
-Matthias
On 12/21/22 2:09 AM, Sagar wrote:
Hi All,
Just as an update, the changes described here:
```
Changes at a high level are:
1) KeyQueryMetada enhanced to have a new method called partitions().
2) Lifting the restriction of a single partition for IQ. Now
Hi All,
Just as an update, the changes described here:
```
Changes at a high level are:
1) KeyQueryMetada enhanced to have a new method called partitions().
2) Lifting the restriction of a single partition for IQ. Now the
restriction holds only for FK Join.
```
are reverted back. As things stan
Hey Matthias,
Actually I had shared the PR link for any potential issues that might have
gone missing. I guess it didn't come out that way in my response. Apologies
for that!
I am more than happy to incorporate any feedback/changes or address any
concerns that are still present around this at thi
It is what it is.
we did have internal discussions on this
We sometimes have the case that a KIP need adjustment as stuff is
discovered during coding. And having a discussion on the PR about it is
fine. -- However, before the PR gets merge, the KIP change should be
announced to verify that
Thanks Matthias,
Well, as things stand, we did have internal discussions on this and it
seemed ok to open it up for IQ and more importantly not ok to have it
opened up for FK-Join. And more importantly, the PR for this is already
merged and some of these things came up during that. Here's the PR l
Ah. Missed it as it does not have a nice "code block" similar to
`StreamPartitioner` changes.
I understand the motivation, but I am wondering if we might head into a
tricky direction? State stores (at least the built-in ones) and IQ are
kinda build with the idea to have sharded data and that a
Hi Mathias,
I did save it. The changes are added under Public Interfaces (Pt#2 about
enhancing KeyQueryMetadata with partitions method) and
throwing IllegalArgumentException when StreamPartitioner#partitions method
returns multiple partitions for just FK-join instead of the earlier decided
FK-Join
I don't see any update on the wiki about it. Did you forget to hit "save"?
Can you also provide some background? I am not sure right now if I
understand the proposed changes?
-Matthias
On 12/6/22 6:36 PM, Sophie Blee-Goldman wrote:
Thanks Sagar, this makes sense to me -- we clearly need add
Thanks Sagar, this makes sense to me -- we clearly need additional changes
to
avoid breaking IQ when using this feature, but I agree with continuing to
restrict
FKJ since they wouldn't stop working without it, and would become much
harder
to reason about (than they already are) if we did enable the
Hi All,
I made a couple of edits to the KIP which came up during the code review.
Changes at a high level are:
1) KeyQueryMetada enhanced to have a new method called partitions().
2) Lifting the restriction of a single partition for IQ. Now the
restriction holds only for FK Join.
Updated KIP:
ht
Thanks Bruno,
Marking this as accepted.
Thanks everyone for their comments/feedback.
Thanks!
Sagar.
On Mon, Sep 12, 2022 at 1:53 PM Bruno Cadonna wrote:
> Hi Sagar,
>
> Thanks for the update and the PR!
>
> +1 (binding)
>
> Best,
> Bruno
>
> On 10.09.22 18:57, Sagar wrote:
> > Hi Bruno,
> >
>
Hi Sagar,
Thanks for the update and the PR!
+1 (binding)
Best,
Bruno
On 10.09.22 18:57, Sagar wrote:
Hi Bruno,
Thanks, I think these changes make sense to me. I have updated the KIP
accordingly.
Thanks!
Sagar.
On Wed, Sep 7, 2022 at 2:16 PM Bruno Cadonna wrote:
Hi Sagar,
I would not dr
Hi Bruno,
Thanks, I think these changes make sense to me. I have updated the KIP
accordingly.
Thanks!
Sagar.
On Wed, Sep 7, 2022 at 2:16 PM Bruno Cadonna wrote:
> Hi Sagar,
>
> I would not drop the support for dropping records. I would also not
> return null from partitions(). Maybe an Optiona
Hi Sagar,
I would not drop the support for dropping records. I would also not
return null from partitions(). Maybe an Optional can help here. An empty
Optional would mean to use the default partitioning behavior of the
producer. So we would have:
- non-empty Optional, non-empty list of integ
Hello Bruno/Chris,
Since these are the last set of changes(I am assuming haha), it would be
great if you could review the 2 options from above so that we can close the
voting. Of course I am happy to incorporate any other requisite changes.
Thanks!
Sagar.
On Wed, Aug 31, 2022 at 10:07 PM Sagar
Thanks Bruno for the great points.
I see 2 options here =>
1) As Chris suggested, drop the support for dropping records in the
partitioner. That way, an empty list could signify the usage of a default
partitioner. Also, if the deprecated partition() method returns null
thereby signifying the defa
+1 to Bruno's concerns about backward compatibility. Do we actually need
support for dropping records in the partitioner? It doesn't seem necessary
based on the motivation for the KIP. If we remove that feature, we could
handle null and/or empty lists by using the default partitioning,
equivalent t
Hi Sagar,
Thank you for the updates!
I do not intend to prolong this vote thread more than needed, but I
still have some points.
The deprecated partition method can return null if the default
partitioning logic of the producer should be used.
With the new method partitions() it seems that it
Thanks Bruno/Chris,
Even I agree that might be better to keep it simple like the way Chris
suggested. I have updated the KIP accordingly. I made couple of minor
changes to the KIP:
1) One of them being the change of return type of partitions method from
List to Set. This is to ensure that in case
Hi,
I am favour of discarding the sugar for broadcasting and leave the
broadcasting to the implementation as Chris suggests. I think that is
the cleanest option.
Best,
Bruno
On 29.08.22 19:50, Chris Egerton wrote:
Hi all,
I think it'd be useful to be more explicit about broadcasting to all
Hi all,
I think it'd be useful to be more explicit about broadcasting to all topic
partitions rather than add implicit behavior for empty cases (empty
optional, empty list, etc.). The suggested enum approach would address that
nicely.
It's also worth noting that there's no hard requirement to add
Hi Sagar,
I do not see an issue with using an empty list together with an empty
Optional. A non-empty Optional that contains an empty list would just
say that there is no partition to which the record should be sent. If
there is no partition, the record is dropped.
An empty Optional might be
Thank you Bruno/Matthew for your comments.
I agree using null does seem error prone. However I think using a singleton
list of [-1] might be better in terms of usability, I am saying this
because the KIP also has a provision to return an empty list to refer to
dropping the record. So, an empty opt
I also concur with this, having an Optional in the type makes it very clear
what’s going on and better signifies an absence of value (or in this case the
broadcast value).
--
Matthew de Detrich
Aiven Deutschland GmbH
Immanuelkirchstraße 26, 10405 Berlin
Amtsgericht Charlottenburg, HRB 209739 B
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 cod
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 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
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 eve
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 p
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 1
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 t
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
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 (
+1 (non binding)
Walker
On Tue, May 31, 2022 at 4:44 AM Sagar 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
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.
34 matches
Mail list logo
