Hi, Looks like a great KIP to me!
I'm late, so I'm only going to comment on the last open point 117. I'm against any fallbacks like "use the default assignor if the custom assignment is invalid", as it's just going to hide bugs. For the 4 cases mentioned by Sophie: 117a) I'd fail immediately here, as it's an implementation bug, and should not lead to a valid consumer group assignment. 117b) Agreed. This is a useful assignment and should be allowed. 117c) This is the tricky case. However, I'm leaning towards not allowing this, unless we have a concrete use case. This will block us from potentially using a single consumer for active and standby tasks in the future. It's easier to drop the restriction later if we have a concrete use case. 117d) Definitely fail immediately, as you said. Cheers, Lucas On Mon, Apr 29, 2024 at 11:13 PM Sophie Blee-Goldman <sop...@responsive.dev> wrote: > > Yeah I think that sums it up well. Either you computed a *possible* > assignment, > or you returned something that makes it literally impossible for the > StreamsPartitionAssignor to decipher/translate into an actual group > assignment, in which case it should just fail > > That's more or less it for the open questions that have been raised so far, > so I just want to remind folks that there's already a voting thread for > this. I cast my vote a few minutes ago so it should resurface in everyone's > inbox :) > > On Thu, Apr 25, 2024 at 11:42 PM Rohan Desai <desai.p.ro...@gmail.com> > wrote: > > > 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 assignment that excludes a task is one such example, and > > Sophie pointed out a good use case for it. I also think it makes sense to > > allow these. It's hard to predict how a user might want to use the custom > > assignor, and its reasonable to expect them to use it with care and not > > hand-hold them. > > 2. cases that are not valid because it is impossible to compute a valid > > consumer group assignment from them. In this case it seems totally > > reasonable to just throw a fatal exception that gets passed to the uncaught > > exception handler. If this case happens then there is some bug in the > > user's assignor and its totally reasonable to fail the application in that > > case. We _could_ try to be more graceful and default to one of the existing > > assignors. But it's usually better to fail hard and fast when there is some > > illegal state detected imo. > > > > On Fri, Apr 19, 2024 at 4:18 PM Rohan Desai <desai.p.ro...@gmail.com> > > wrote: > > > > > Bruno, I've incorporated your feedback into the KIP document. > > > > > > On Fri, Apr 19, 2024 at 3:55 PM Rohan Desai <desai.p.ro...@gmail.com> > > > 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 different - that we might want > > >> to wrap the return type for `assign` in a class so that its easily > > >> extensible. This makes sense to me. Whether we do that or not, we can > > have > > >> the return type be a Set instead of a Map as well. > > >> > > >> re 6: Yes, it's a callback that's called with the final assignment. I > > >> like your suggested name. > > >> > > >> On Fri, Apr 5, 2024 at 12:17 PM Rohan Desai <desai.p.ro...@gmail.com> > > >> wrote: > > >> > > >>> 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 > > >>> final assignment. I'll add that. > > >>> re4: I like the new `NodeAssignment` type. I'll update the KIP with > > that. > > >>> > > >>> On Thu, Nov 9, 2023 at 11:18 PM Rohan Desai <desai.p.ro...@gmail.com> > > >>> wrote: > > >>> > > >>>> 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 > > >>>> partitions->tasks mapping is formed? > > >>>> You're right that this is out of place. I've removed this method as > > >>>> it's not needed by the task assignor. > > >>>> > > >>>> > 2. Just on the API layering itself: it feels a bit weird to have the > > >>>> three built-in functions (defaultStandbyTaskAssignment etc) sitting in > > >>>> the ApplicationMetadata class. If we consider them as some default > > util > > >>>> functions, how about introducing moving those into their own static > > util > > >>>> methods to separate from the ApplicationMetadata “fact objects” ? > > >>>> Agreed. Updated in the latest revision of the kip. These have been > > >>>> moved to TaskAssignorUtils > > >>>> > > >>>> > 3. I personally prefer `NodeAssignment` to be a read-only object > > >>>> containing the decisions made by the assignor, including the > > >>>> requestFollowupRebalance flag. For manipulating the half-baked results > > >>>> inside the assignor itself, maybe we can just be flexible to let > > users use > > >>>> whatever struts / their own classes even, if they like. WDYT? > > >>>> Agreed. Updated in the latest version of the kip. > > >>>> > > >>>> > 1. For the API, thoughts on changing the method signature to return > > a > > >>>> (non-Optional) TaskAssignor? Then we can either have the default > > >>>> implementation return new HighAvailabilityTaskAssignor or just have a > > >>>> default implementation class that people can extend if they don't > > want to > > >>>> implement every method. > > >>>> Based on some other discussion, I actually decided to get rid of the > > >>>> plugin interface, and instead use config to specify individual plugin > > >>>> behaviour. So the method you're referring to is no longer part of the > > >>>> proposal. > > >>>> > > >>>> > 3. Speaking of ApplicationMetadata, the javadoc says it's read only > > >>>> but > > >>>> theres methods that return void on it? It's not totally clear to me > > how > > >>>> that interface is supposed to be used by the assignor. It'd be nice > > if we > > >>>> could flip that interface such that it becomes part of the output > > instead > > >>>> of an input to the plugin. > > >>>> I've moved those methods to a util class. They're really utility > > >>>> methods the assignor might want to call to do some default or > > optimized > > >>>> assignment for some cases like rack-awareness. > > >>>> > > >>>> > 4. We should consider wrapping UUID in a ProcessID class so that we > > >>>> control > > >>>> the interface (there are a few places where UUID is directly used). > > >>>> I like it. Updated the proposal. > > >>>> > > >>>> > 5. What does NodeState#newAssignmentForNode() do? I thought the > > >>>> point was > > >>>> for the plugin to make the assignment? Is that the result of the > > >>>> default logic? > > >>>> It doesn't need to be part of the interface. I've removed it. > > >>>> > > >>>> > re 2/6: > > >>>> > > >>>> I generally agree with these points, but I'd rather hash that out in a > > >>>> PR than in the KIP review, as it'll be clearer what gets used how. It > > seems > > >>>> to me (committers please correct me if I'm wrong) that as long as > > we're on > > >>>> the same page about what information the interfaces are returning, > > that's > > >>>> ok at this level of discussion. > > >>>> > > >>>> On Tue, Nov 7, 2023 at 12:03 PM Rohan Desai <desai.p.ro...@gmail.com> > > >>>> wrote: > > >>>> > > >>>>> 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 > > more > > >>>>> detail on why we think this is a useful addition. Thanks in advance > > for > > >>>>> your feedback! > > >>>>> > > >>>>> Best Regards, > > >>>>> > > >>>>> Rohan > > >>>>> > > >>>>> > >
