117: as Sophie laid out, there are two cases here right:
1. cases that are considered invalid by the existing assignors but are
still valid assignments in the sense that they can be used to generate a
valid consumer group assignment (from the perspective of the consumer group
protocol). An
Thanks for the details!
107: I like `numProcessingThreads()` proposal.
108(a): it was not really a concern, but it was rather a question if we
could/should simplify it, so it's easier to implement a custom task
assignor. But if we believe that it's an integral component, I am fine
with
104. Fair enough -- also happy to defer to Rohan on this (or Bruno if he
feels super strongly)
107. That's a good point . Ultimately the task load should reflect the
processing capacity, and that's something that will exist in both the new
and old threading model. I like #processingCapacity for
104: I also don't feel super strong about it. Not sure if
`onAssignment()` might overload the name in a confusing way? In the end,
when the method is called, we don't assign anything? -- Guess, I am fine
with whatever Rohan picks as a name from the suggestions we have so far.
107: Did not
Now to respond to Matthias:
FYI, I'm following the numbering scheme from your email but added to
mark responses with further questions or feedback and/or aren't yet
addressed in the KIP and need to be followed up on. You can more or less
just skip over the ones without stars to save time
Responding to Bruno first:
(1) I actually think "KafkaStreams" is exactly right here -- for the reason
you said, ultimately this is describing a literal instance of the
"KafkaStreams" class. Glad we hashed this out! (I saw that Rohan went with
StreamsClient but i also prefer KafkaStreams)
(4)
One more thing. It might be good to clearly call out, which interfaced a
user would implement, vs the other ones Kafka Streams implements and
TaskAssignor only uses.
My understanding is, that users would implement `TaskAssignor`,
`TaskAssignment`, and `StreamsClientAssignment`.
For
Great KIP. I have some minor comments/questions:
100 The KIP says: "In the future, additional plugins can use the same
partition.assignor prefix". What does this mean?
101 (nit) The KIP says: "Note that the thread-level assignment will
remain an un-configurable internal implementation
Bruno, I've incorporated your feedback into the KIP document.
On Fri, Apr 19, 2024 at 3:55 PM Rohan Desai wrote:
> Thanks for the feedback Bruno! For the most part I think it makes sense,
> but leaving a couple follow-up thoughts/questions:
>
> re 4: I think Sophie's point was slightly
Thanks for the feedback Bruno! For the most part I think it makes sense,
but leaving a couple follow-up thoughts/questions:
re 4: I think Sophie's point was slightly different - that we might want to
wrap the return type for `assign` in a class so that its easily extensible.
This makes sense to
Hi Sophie,
Thanks for the clarifications!
(1)
What about replacing Node* with KafkaStreams* or StreamsClient*? I
prefer KafkaStreams* since that class represents the Kafka Streams
client. I am also fine with KafkaStreamsClient*. I really would like to
avoid introducing a new term in Kafka
Thanks Bruno! I can provide a bit of context behind some of these
decisions but I just want to say up front that I agree with every single one
of your points, though I am going to push back a bit on the first one.
[1] The idea here is to help avoid some confusion around the overloaded
term
Hi,
sorry, I am late to the party.
I have a couple of comments:
(1)
I would prefer Client* instead of Node* in the names. In Kafka Streams
we do not really have the concept of node but we have the concept of
client (admittedly, we sometimes also use instance). I would like to
avoid
Cool, looks good to me!
Seems like there is no further feedback, so maybe we can start to call for
a vote?
However, since as noted we are setting aside time to discuss this during
the sync next Thursday, we can also wait until after that meeting to
officially kick off the vote.
On Fri, Apr 5,
Thanks for the feedback Sophie!
re1: Totally agree. The fact that it's related to the partition assignor is
clear from just `task.assignor`. I'll update.
re3: This is a good point, and something I would find useful personally. I
think its worth adding an interface that lets the plugin observe the
Before I go into my own feedback, which I know not everyone has the time to
read, I just want to announce that we plan to go over this KIP during the
next Streams project sync on April 11. If you are interested in joining the
discussion and need an invitation, just reach out to me in a separate
Hi Rohan,
I took another look at the updated wiki page and do not have any major
questions. Regarding returning a plugin object v.s. configuring a
plugin object, I do not have a strong opinion except that the latter
seems more consistent with existing patterns. Just curious, any other
motivations
Thanks for the feedback so far! I think pretty much all of it is
reasonable. I'll reply to it inline:
> 1. All the API logic is granular at the Task level, except the
previousOwnerForPartition func. I’m not clear what’s the motivation behind
it, does our controller also want to change how the
Hello Rohan,
Thanks for the KIP! Thoughts below (seems I have similar comments to
Guozhang, but I had already written this before reading his reply haha!).
They're basically all minor suggestions for improvements, I wouldn't
consider any of them blocking.
1. For the API, thoughts on changing the
Hello Rohan,
Thanks for the KIP! Overall it looks very nice. Just some quick thoughts :
1. All the API logic is granular at the Task level, except the
previousOwnerForPartition func. I’m not clear what’s the motivation
behind it, does our controller also want to change how the
partitions->tasks
Hello All,
I'd like to start a discussion on KIP-924 (
https://cwiki.apache.org/confluence/display/KAFKA/KIP-924%3A+customizable+task+assignment+for+Streams)
which proposes an interface to allow users to plug into the streams
partition assignor. The motivation section in the KIP goes into some
21 matches
Mail list logo
